> 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/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
