[PATCH] D87118: Add an explicit toggle for the static analyzer in clang-tidy

[Nico Weber via Phabricator via cfe-commits]

thakis added inline comments.


================
Comment at: clang-tools-extra/CMakeLists.txt:4
+option(CLANG_TIDY_ENABLE_STATIC_ANALYZER
+  "Include static analyzer checks in clang-tidy" ON)
+
----------------
hans wrote:
> Should this default to CLANG_ENABLE_STATIC_ANALYZER instead of ON?
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


================
Comment at: clang/lib/CMakeLists.txt:24
 add_subdirectory(IndexSerialization)
-if(CLANG_ENABLE_STATIC_ANALYZER)
-  add_subdirectory(StaticAnalyzer)
----------------
hans wrote:
> Why does removing the condition here work?
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.


================
Comment at: llvm/utils/gn/secondary/clang-tools-extra/clang-tidy/BUILD.gn:18
+  } else {
+    values += [ "CLANG_TIDY_ENABLE_STATIC_ANALYZER=" ]
+  }
----------------
hans wrote:
> Why not =0?
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.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87118/new/

https://reviews.llvm.org/D87118