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

Add clang-tidy to buildsystem checks #1063

Closed
wants to merge 1 commit into from

Conversation

kev946
Copy link
Contributor

@kev946 kev946 commented Oct 20, 2018

Fixes #1060

Copy link
Member

@TheJJ TheJJ left a comment

Choose a reason for hiding this comment

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

Thank you! I think clang-tidy needs to be aware of the build system (https://blog.kitware.com/static-checks-with-cmake-cdash-iwyu-clang-tidy-lwyu-cpplint-and-cppcheck/ and http://mariobadr.com/using-clang-tidy-with-cmake-36.html).

What we need to think about is how/when we merge this. Because previously, we have to fix all the issues it finds. And we should never ever have a non-passing CI state.
So my proposal is to not enable the check by default, so we can gradually fix all the problems and after that, we enable it so that Kevin fails.

buildsystem/codecompliance/clangtidy.py Show resolved Hide resolved
@TheJJ TheJJ added area: buildsystem Related to our cmake/python buildsystem nice new thing ☺ A new feature that was not there before labels Nov 5, 2018
@TheJJ TheJJ self-assigned this Mar 4, 2019
@simonsan
Copy link
Contributor

simonsan commented Sep 25, 2019

I want to let this here:
https://github.com/KDE/clazy

clazy is a compiler plugin which allows clang to understand Qt semantics. You get more than 50 Qt related compiler warnings, ranging from unneeded memory allocations to misusage of API, including fix-its for automatic refactoring.

Here is a list with checks:
https://github.com/KDE/clazy#list-of-checks

@simonsan
Copy link
Contributor

simonsan commented Sep 25, 2019

Which of these checks do we want to focus on?
I would propose a selection of the following with another clang-tidy-parameter stating explicitly -extra-arg=-std=c++17. clang-diagnostic-* gives probably the most warnings, so we should add these checks in the end when we are fine with the rest.

EDIT1: Excluded some spammy commands from the list.

cppcoreguidelines-* | Checks related to C++ Core
clang-analyzer-* | Clang Static Analyzer checks.
performance-* | Checks that target performance-related issues.
portability-* | Checks that target portability-related issues that don’t relate to any particular coding style.
misc-* | Checks that we didn’t have a better category for.
modernize-* | Checks that advocate usage of modern (currently “modern” means “C++11”) language constructs.
hicpp-* | Checks related to High Integrity C++ Coding Standard.
bugprone-* | Checks that target bugprone code constructs.
cert-* | Checks related to CERT Secure Coding Guidelines.

EDIT2: With these commands we are probably fine and should be not having so many errors.

 invocation = ['clang-tidy', '-checks=cppcoreguidelines-*,clang-analyzer-*,bugprone-*,performance-*, ' +
                  'portability-*,misc-*,modernize-*,hicpp-*,cert-*,llvm-twine-local, ' + 
                  'llvm-prefer-isa-or-dyn-cast-in-conditionals, llvm-prefer-register-over-unsigned, ' + 
                  'google-readability-casting -extra-arg=-std=c++17']

The problem is just, that it doesn't find the header files and this throws mostly (afais) the errors.
So we need to tell CMake to export the compiler commands with CMAKE_EXPORT_COMPILE_COMMANDS which we can import with the flag -p for the check. I'll try that out later.

https://stackoverflow.com/a/39596822

@simonsan
Copy link
Contributor

Nice work, I close this due to inactivity and continue working on it in #1157. Feel free to reopen if you add commits to this PR. Cheers.

@simonsan simonsan closed this Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: buildsystem Related to our cmake/python buildsystem nice new thing ☺ A new feature that was not there before
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add clang-tidy to buildsystem checks
3 participants