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

Branch

clang-tidy

Lint

No Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 218
Build 218: arc lint + arc unit

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