Page MenuHomeHEPForge

Add CMake options for enabling clang-tidy static analysis checks during build
ClosedPublic

Authored by tlatham on Apr 7 2022, 2:59 PM.

Details

Summary

Add the following CMake options:

  • EVTGEN_RUN_CLANG_TIDY
  • EVTGEN_CLANG_TIDY_FIX
  • EVTGEN_CLANG_TIDY_CHECKS

to control running of clang-tidy static analysis checks during build.

  • The first toggles whether the checks should be run.
  • The second toggles whether auto-fix should be enabled.
  • The third allows customisation of the exact checks to be run.

The checks currently default to a single check:
modernize-use-nullptr
just to test that things are working as expected.

Going forward, we can use the large set of possible checks to gradually improve the code.

Eventually we can establish a set of checks to run in the CI to test new/changed code.

See:
https://clang.llvm.org/extra/clang-tidy/index.html
for general info on clang-tidy, and
https://clang.llvm.org/extra/clang-tidy/checks/list.html
for the list of all currently supported checks.

Test Plan
  • Enable the EVTGEN_RUN_CLANG_TIDY option when configuring the build and see that the modernize-use-nullptr check runs.
  • Enable additionally the EVTGEN_CLANG_TIDY_FIX option and see that the code is auto-fixed during the checks.
  • Set a variety of values for list of checks and see that the right checks run.

Diff Detail

Repository
rEVTGEN evtgen
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tlatham changed the visibility from "All Users" to "Public (No Login Required)".Apr 7 2022, 3:01 PM
tlatham changed the edit policy from "All Users" to "Restricted Project (Project)".
tlatham added a project: Restricted Project.
tlatham published this revision for review.Apr 7 2022, 3:03 PM
tlatham edited the summary of this revision. (Show Details)
tlatham edited the test plan for this revision. (Show Details)
tlatham added reviewers: jback, kreps.

Looks good to me, but of coarse I did not check full details of clang-tidy configuration file. I suspect we might need to cleanup existing code as well as we turn on more checks.

This revision is now accepted and ready to land.Apr 29 2022, 1:19 PM

Thanks Michal.
The config file is mostly just the default values with the only customisation coming in the first 5 lines:

  • line 1 to run only the modernize-use-nullptr check
  • line 3 to ensure that it warns about things in our header files
  • line 5 so that it uses our .clang-format file for reformatting after any auto-applied changes

I have no doubt that the existing code will need cleaning up as we enable more checks. This is why I say "eventually" about having this run automatically, otherwise we'll just be swamped with warnings. A point for subsequent discussion will be what checks we want to use and in what order to start to gradually enable them and fix stuff as we go. I'll create a task where we can have such a discussion.

I'm not an expert on cmake but all of the changes look OK and we can add more checks/features as time goes on. Let us go ahead and add this to the repository.