[PATCH] [clang-tidy] Support for Static Analyzer plugins

[Alexander Kornienko]

> Small bump, just in case this got forgotten.


Yes, I actually forgot to reply. Feel free to ping me each week or so, in case I don't reply.

Now to the patch itself. I have a number of concerns, please see the comments inline.


================
Comment at: clang-tidy/ClangTidy.cpp:61
@@ -57,10 +60,3 @@
 
-static StringRef StaticAnalyzerChecks[] = {
-#define GET_CHECKERS
-#define CHECKER(FULLNAME, CLASS, DESCFILE, HELPTEXT, GROUPINDEX, HIDDEN)       \
-  FULLNAME,
-#include "../../../lib/StaticAnalyzer/Checkers/Checkers.inc"
-#undef CHECKER
-#undef GET_CHECKERS
-};
+static std::vector<std::string> StaticAnalyzerChecks;
 
----------------
It was fine to have this as a global variable while it was a compile-time constant. But it's a bad idea to use a global mutable variable to pass configuration to the library. Actually, I'd leave the compile-time checks list as a fallback, and only override it if clang-tidy is instructed to use a static analyzer plugin.

================
Comment at: clang-tidy/ClangTidy.cpp:409
@@ -412,1 +408,3 @@
 
+void populateStaticAnalyzerCheckerList(StringRef Plugin, const char **Argv) {
+  IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts(new DiagnosticOptions());
----------------
Is the only useful effect of this function to fill the StaticAnalyzerChecks variable? It's bad on its own as any other interface which only useful effect is side-effect.

AFAIU, you still rely on the compilation arguments to instruct the static analyzer that it needs to load a plugin? That doesn't sound like a good idea either for multiple reasons, most importantly that:
  1. This requires modification of the compilation arguments in the build system in order to run an analysis implemented in a plugin. Looks like an overkill and a totally wrong way of doing this.
  2. Compile arguments will affect what is loaded and executed by clang-tidy, which also seems wrong. If it is already the case, it was definitely unintended and we need to fix this instead of relying on this.
  3. 1 and 2 also mean that for correct usage clang-tidy's -plugin-path option will always need to be in sync with the -load options in the compilation database (read: in the build system). Extremely brittle and confusing.



================
Comment at: clang-tidy/ClangTidy.cpp:421
@@ +420,3 @@
+    CommandLine.push_back("-Xclang");
+    CommandLine.push_back("-load");
+    CommandLine.push_back("-Xclang");
----------------
BTW, does this option rely on the plugin support turned on at build time? If yes, we need to make most of this code #ifdefed out using the same preprocessor macros.

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:149
@@ -148,1 +148,3 @@
 
+static cl::opt<std::string> PluginPath(
+    "plugin-path",
----------------
Does static analyzer support only one plugin at a time?

Also, it _may_ be better to put this into ClangTidyOptions, but I'm not sure yet. Upsides are that then we'd avoid modification of most other interfaces. Obvious downsides are that this option only makes sense for the command-line front-end.

Can you explain your use case in more detail?

================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:270
@@ -263,2 +269,3 @@
 
+
 static int clangTidyMain(int argc, const char **argv) {
----------------
nit: Remove the empty line.

================
Comment at: clang-tidy/tool/Makefile:19
@@ -16,2 +18,3 @@
 
+endif
 include $(CLANG_LEVEL)/../../Makefile.config
----------------
nit: This should be above the empty line.

http://reviews.llvm.org/D9555

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/