Page MenuHomePhabricator

Add an explicit toggle for the static analyzer in clang-tidy
ClosedPublic

Authored by thakis on Sep 3 2020, 5:07 PM.

Details

Reviewers
hans
NoQ
Summary

Instead of using CLANG_ENABLE_STATIC_ANALYZER for use of the
static analyzer in both clang and clang-tidy, add a second
toggle CLANG_TIDY_ENABLE_STATIC_ANALYZER.

This allows enabling the static analyzer in clang-tidy while
disabling it in clang.

Diff Detail

Event Timeline

thakis created this revision.Sep 3 2020, 5:07 PM
thakis requested review of this revision.Sep 3 2020, 5:07 PM
hans added a comment.Sep 7 2020, 2:15 AM

Cool!

clang-tools-extra/CMakeLists.txt
4

Should this default to CLANG_ENABLE_STATIC_ANALYZER instead of ON?

clang/lib/CMakeLists.txt
24

Why does removing the condition here work?

llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/BUILD.gn
18

Why not =0?

Szelethus added a reviewer: NoQ.EditedSep 7 2020, 3:26 AM

@NoQ in particular has been working hard on the common infrastructure in between the static analyzer and clang-tidy, I'll add him :)

thakis added inline comments.Sep 9 2020, 11:21 AM
clang-tools-extra/CMakeLists.txt
4

I had thought about it but decided against it:

  • They both default to on so it doesn't matter for the default
  • If you turn off CLANG_ENABLE_STATIC_ANALYZER it's not clear you also want to disable it for clang-tidy
  • It mixes cmake options from two different directories which can be a bit hairy
  • Having the two toggles act independently is easier to understand
clang/lib/CMakeLists.txt
24

As far as I understand, it just impacts which targets CMake generates. clang/lib/FrontendTool/CMakeLists.txt only adds the dep on clangStaticAnalyzerFrontend if CLANG_ENABLE_STATIC_ANALYZER is set, so this doesn't change what gets built for "clang". If you build all targets, this will now always build the analyzer sources and I suppose it makes it a bit easier to accidentally depend on clangStaticAnalyzerFrontend , but I don't know of another way to be able to link this into clang-tidy when it's not built at all over here.

llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/BUILD.gn
18

Because it's a #cmakedefine01, and the string "0" counts as truthy. See http://llvm-cs.pcc.me.uk/utils/gn/build/write_cmake_config.py#15

(That should probably print an error if a #cmakedefine gets a "0" argument since that's basically never what you want.)

thakis added inline comments.Sep 9 2020, 11:32 AM
llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/BUILD.gn
18

Reading that script more, a better answer is "because the behavior of that script isn't quite right". It should treat '0' as false-y and all the gn files should use =0 where they currently use =. I'll make a separate CL for that, but for now this is consistent with everything that's there.

thakis added inline comments.Sep 9 2020, 11:35 AM
llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/BUILD.gn
18

...oh looks like I added a check at least in rG252c4dce6189, but llvm-cs seems to be very stale?

hans accepted this revision.Sep 10 2020, 2:15 AM

lgtm

This revision is now accepted and ready to land.Sep 10 2020, 2:15 AM
thakis closed this revision.Sep 10 2020, 9:21 AM
arichardson added inline comments.
clang/lib/CMakeLists.txt
24

I just noticed that my builds (just a plain ninja) are compiling all static analyzer sources. I am explicitly passing -DCLANG_ENABLE_STATIC_ANALYZER=OFF to cmake (and not building any clang-tools-extra).
I feel like this was not happening before so it's possible there was some CMake change more recently that is now causing this behaviour.

I tried setting EXCLUDE_FROM_ALL (https://cmake.org/cmake/help/latest/prop_tgt/EXCLUDE_FROM_ALL.html) on the directories and targets but that didn't fix the issue for me.

How about changing the condition to
if (CLANG_TIDY_ENABLE_STATIC_ANALYZER OR CLANG_ENABLE_STATIC_ANALYZER)? Or will that not work since CLANG_TIDY_ENABLE_STATIC_ANALYZER isn't defined yet?

Herald added a project: Restricted Project. · View Herald TranscriptFri, Sep 10, 9:17 AM
arichardson added inline comments.Fri, Sep 10, 10:02 AM
clang/lib/CMakeLists.txt
24

Possible solution: D109611