Page MenuHomePhabricator

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

Authored by thakis on Thu, Sep 3, 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.Thu, Sep 3, 5:07 PM
thakis requested review of this revision.Thu, Sep 3, 5:07 PM
hans added a comment.Mon, Sep 7, 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.EditedMon, Sep 7, 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.Wed, Sep 9, 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.Wed, Sep 9, 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.Wed, Sep 9, 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.Thu, Sep 10, 2:15 AM

lgtm

This revision is now accepted and ready to land.Thu, Sep 10, 2:15 AM
thakis closed this revision.Thu, Sep 10, 9:21 AM