nuttyartist / notes

Fast and beautiful note-taking app written in C++. Write down your thoughts.
https://www.get-notes.com
Mozilla Public License 2.0
3.6k stars 316 forks source link

Added .clang-tidy file #660

Closed zjeffer closed 6 months ago

zjeffer commented 6 months ago

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)

guihkx commented 6 months ago

This looks very promising!

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

zjeffer commented 6 months ago

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

https://github.com/nuttyartist/notes/assets/4633209/832f7b86-0257-4982-97f2-aea3c0e96db0

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@guihkx Found two ways to make it much faster:

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.