clang-tidy

[tjhei]

@masterleinad and I have been playing with clang-tidy and we have a couple questions:

1. What clang-tidy checks should we use: mpi-*,performance-*, anything else?
2. We are currently at around ~10 warnings (excluding bundled/*) with some places we probably don't want to fix. Should we have as our goal to always have 0 warnings? You can disable a warning by putting // NOLINT in the specific line of code. Are people okay with this to get down to zero?
3. Should we run this a) nightly and report to cdash, b) as CI at every pull request with a failed check if we get >0 warnings c) only on demand (I am prepping a script with our settings to be put into contrib/utilities/). I am accepting hardware donations for b) ;-)

For context: a clang-tidy run takes about the same time as a full compile run for the whole code base.

[davydden]

Are people okay with this to get down to zero?

I don't know the examples, but I would probably not do that and keep those warnings.

as CI at every pull request with a failed check if we get >0 warnings

that would be ideal, of course.

[tjhei]

@davydden you can not have both, I think: either we use CI to enforce 0 clang-tidy warnings or we are okay with leaving some warnings in the code.

[davydden]

@tjhei

I thought we can check if we get new warnings.

EDIT: But if that's not possible, I agree that it's ok to silence a few existing once.

[tjhei]

oh, that is a bit more difficult to do.

[tamiko]

@tjhei How did you enable clang-tidy? By setting the CMake target property?

[tjhei]

@tamiko Option 1: run-clang-tidy.py based on output from CMAKE_EXPORT_COMPILE_COMMANDS=ON (you can just run clang-tidy without having to compile the source code). I will have a PR with the script that does it soon.

Option 2: enable with
set_target_properties(some_target PROPERTIES CXX_CLANG_TIDY "bla config")
with cmake 3.6+. Problem: need to modify targets and I don't think I want to do that.

Option 3: use -DCMAKE_CXX_CLANG_TIDY="bla config". Simple but requires you to also compile the codebase. (cmake 3.6+)

[tjhei]

Option 4: we add an option "DEAL_II_WITH_CLANG_TIDY=ON" that sets CMAKE_CXX_CLANG_TIDY to the correct configuration.

[tamiko]

Honestly, I prefer option 1 - just setting the cmake variable on the command line. There is so much we would have to put into a proper feature configuration (check for compiler, check for executable, etc.) - further we would have to decide on a set of clang-tidy options.

[tjhei]

I don't understand. Do you mean 1 or 4?

further we would have to decide on a set of clang-tidy options.

That is what we want to do anyways. Without agreeing on a set of options, it is impossible to setup and use clang-tidy by yourself.

[tjhei]

There is so much we would have to put into a proper feature configuration

Huh? For which option would you need to do that? All of the options above are 5-10 lines of bash or CMake code.

[tamiko]

@tjhei My comment sounded a bit stronger than I expected.

I would prefer a bash script (in contrib/utilities) that sets up the build directory with -DCMAKE_EXPORT_COMPILE_COMMANDS=ON, runs the expand_all_instantiations target and finally calls clang-tidy :-)

[tjhei]

oh, that makes sense now! ;-)

Patch incoming soon.

[masterleinad]

The checks we are currently using are:

• mpi-*,
• modernize-use-emplace,
• performance-* (except for performance-inefficient-string-concatenation)

What else should we add?

[tjhei]

There is now a nightly clang-tidy run that is submitting to cdash. The clang-tidy messages show up as warnings. Example: https://cdash.kyomu.43-1.org/viewBuildError.php?type=1&buildid=15986