Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added .clang-tidy file #660

Merged
merged 2 commits into from
Dec 13, 2023
Merged

Added .clang-tidy file #660

merged 2 commits into from
Dec 13, 2023

Conversation

zjeffer
Copy link
Collaborator

@zjeffer zjeffer commented Dec 9, 2023

I also added this for the Waybar repo (see discussion here for more info on what it is).

Later we could also add a GitHub action that checks whether new code adheres to these clang-tidy checks (I'm testing this here: https://github.com/zjeffer/Waybar/pull/1/files)

@zjeffer zjeffer requested review from guihkx and nuttyartist December 9, 2023 12:50
-readability-redundant-member-init,
-readability-redundant-string-init,
-readability-identifier-length,
CheckOptions:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also added naming checks, so we all use the same naming conventions. I chose these based on what we currently use most often.
Let me know if any of these need to change.

@guihkx
Copy link
Collaborator

guihkx commented Dec 9, 2023

This looks very promising!

Can I test this locally? If so, what commands should I use exactly?

@zjeffer
Copy link
Collaborator Author

zjeffer commented Dec 9, 2023

In vscode, with the clangd extension, you can see this in the editor itself:

2023-12-09.15-01-41.mp4

You can hover over the warning and most of the time there is an automatic fix you can apply.


You can also manually run clang-tidy in a terminal:

find src -name '*.cpp' -o -name '*.hpp' | xargs clang-tidy --config-file=./.clang-tidy.

You need to install clang-tidy on your system (using your package installer of choice)

@guihkx
Copy link
Collaborator

guihkx commented Dec 9, 2023

The VSCode integration looks neat!

find src -name '*.cpp' -o -name '*.hpp' | xargs clang-tidy --config-file=./.clang-tidy.

Thanks, but I think that's probably not the "right way" to do it?

Because I'm getting a lot of errors running it that way, including nonsensical ones like:

/home/gui/dev/notes/src/aboutwindow.h:4:10: error: 'QDialog' file not found [clang-diagnostic-error]
#include <QDialog>
         ^~~~~~~~~

@zjeffer
Copy link
Collaborator Author

zjeffer commented Dec 9, 2023

You can pass the compile_commands.json file:

find ./src -regex ".*\.\(c\|cpp\|h\|hpp\)$" | xargs clang-tidy --config-file ./.clang-tidy -p ./build/compile_commands.json

It's a lot slower when doing this, though, because it has to parse all included Qt header files as well. But it looks a lot better.
It seems to still have trouble with private Qt headers, though.

For example, in src/nodetreeview_p.h, we include <QtWidgets/private/qabstractitemview_p.h>, which reports it cannot find this:

/usr/include/qt6/QtWidgets/6.6.1/QtWidgets/private/qabstractitemview_p.h:133:32: error: no type named 'DropIndicatorPosition' in 'QAbstractItemModel' [clang-diagnostic-error]
    virtual QAbstractItemView::DropIndicatorPosition position(const QPoint &pos, const QRect &rect, const QModelIndex &idx) const;
            ~~~~~~~~~~~~~~~~~~~^

I also see this error in vscode.

@guihkx
Copy link
Collaborator

guihkx commented Dec 11, 2023

You can pass the compile_commands.json file:

Alright, so I didn't know how to generate that file, but simply invoking CMake with -DCMAKE_EXPORT_COMPILE_COMMANDS=ON did the trick.

Perhaps we can add it to CMakeLists.txt so it can be generated by default?

It's a lot slower when doing this, though, because it has to parse all included Qt header files as well. But it looks a lot better.

It too a while to finish, indeed, but the amount of warnings and errors I got at the end still seem too high...?

$ find ./src -regex '.*\.\(c\|cpp\|h\|hpp\)$' | xargs clang-tidy --config-file ./.clang-tidy -p ./build/compile_commands.json
[... very long output ...]
1139005 warnings and 41 errors generated.
Error while processing /home/gui/dev/notes/src/nodedata.h.
error: too many errors emitted, stopping now [clang-diagnostic-error]

Or is that expected?

@zjeffer
Copy link
Collaborator Author

zjeffer commented Dec 11, 2023

Perhaps we can add it to CMakeLists.txt so it can be generated by default?

This should indeed be added to CMakeLists.txt by default. I think the vscode CMake Tools extension already does that for me when I configure, which is why I didn't notice it wasn't included.

@zjeffer
Copy link
Collaborator Author

zjeffer commented Dec 11, 2023

It too a while to finish, indeed, but the amount of warnings and errors I got at the end still seem too high...?

$ find ./src -regex '.*.(c|cpp|h|hpp)$' | xargs clang-tidy --config-file ./.clang-tidy -p ./build/compile_commands.json
[... very long output ...]
1139005 warnings and 41 errors generated.
Error while processing /home/gui/dev/notes/src/nodedata.h.
error: too many errors emitted, stopping now [clang-diagnostic-error]

Or is that expected?

I also get roughly that amount of warnings, but it doesn't stop with too many errors emitted, that's odd. I'll run it again.

According to this stackoverflow thread: https://stackoverflow.com/questions/64360160/what-does-clang-tidy-suppressed-x-warnings-mean, those warnings are (supposed to be) suppressed because they come from external code (standard library, qt sources, other libraries, etc.). We should try to find a way to skip checking those files (if possible), I think it'll run a lot faster.

@zjeffer
Copy link
Collaborator Author

zjeffer commented Dec 11, 2023

Nevermind, I do get the too many errors emitted, stopping now [clang-diagnostic-error] error, here's my full output:

clang-tidy-warnings.txt

@zjeffer
Copy link
Collaborator Author

zjeffer commented Dec 13, 2023

@guihkx Found two ways to make it much faster:

  • the clang package includes both /usr/bin/clang-tidy and /usr/bin/run-clang-tidy. The latter runs clang-tidy in parallel.
  • instead of finding first and then running clang-tidy on every file using xargs, you can run find in $()

It still outputs the warning counts like:

1139005 warnings and 41 errors generated.
Error while processing /home/gui/dev/notes/src/nodedata.h.
error: too many errors emitted, stopping now [clang-diagnostic-error]

but you can disable that with -quiet.


The command then becomes:

run-clang-tidy -p ./build/ -config-file ./.clang-tidy $(find ./src -regex '.*\.\(cpp\|hpp\|h\|c\)$') -quiet

New output: clang-tidy-warnings2.txt

Either way, clang-tidy is not really supposed to be used like this, you're supposed to set up your IDE with clang as a language server and then enable clang-tidy in the clang options.

Copy link
Collaborator

@guihkx guihkx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way, clang-tidy is not really supposed to be used like this, you're supposed to set up your IDE with clang as a language server and then enable clang-tidy in the clang options.

Yeah, using it like that makes more sense. But it's still nice to know there's a way to run with only a command.

@zjeffer zjeffer merged commit 1f83aa5 into master Dec 13, 2023
17 checks passed
@zjeffer zjeffer deleted the feat/zjeffer/clang-tidy branch December 13, 2023 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants