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
Branch
clang-tidy
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 213
Build 213: arc lint + arc unit

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.