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

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

Details

Reviewers

jback
kreps

Commits

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

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 created this revision.Apr 7 2022, 2:59 PM

Harbormaster completed remote builds in B213: Diff 349.Apr 7 2022, 2:59 PM

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.

tlatham mentioned this in T181: Use clang-tidy static analysis checks.Apr 29 2022, 2:22 PM

Rebased on master

Harbormaster completed remote builds in B218: Diff 356.May 11 2022, 2:57 PM

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.

Closed by commit rEVTGEN9c03ce019017: Add CMake options for enabling clang-tidy static analysis checks during build (authored by tlatham). · Explain WhyMay 11 2022, 4:51 PM

This revision was automatically updated to reflect the committed changes.

tlatham added a commit: rEVTGEN9c03ce019017: Add CMake options for enabling clang-tidy static analysis checks during build.

tlatham mentioned this in D83: Fix double counting of channels with K0 and charmonia for B0 decays.May 11 2022, 4:56 PM