================
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;
----------------
xazax.hun wrote:
> alexfh wrote:
> > 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.
> It is a good idea to fall back to the checker list generated from header
> files.
>
> What do you think, where should I put the mutable vector? In ClangTidyOptions?
See the comment below.
Also, please make this variable an array of StringRefs again.
================
Comment at: clang-tidy/ClangTidy.cpp:409
@@ -412,1 +408,3 @@
+void populateStaticAnalyzerCheckerList(StringRef Plugin, const char **Argv) {
+ IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts(new DiagnosticOptions());
----------------
xazax.hun wrote:
> alexfh wrote:
> > 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.
> >
> >
> Yes, the only effect of this function is to initialize the checker list. What
> else would you prefer? Once the mutable list is the member of
> ClangTidyOptions, I could make this function a method.
>
> With plugin-path argument, the command line of compilation do not need to be
> modified from the user's point of view. I only forgot to update the test
> case. Once the plugin path is given in the command line, this method should
> just work with any kind of unmodified compilation database.
>
> The reason why it is not needed to add the -load argument to the command line:
>
> This function loads the plugin which is kept in the memory for the whole
> runtime. Upon plugin load, the checkers are registered. So even though the
> load argument is not given to the frontend action that does the checking, the
> plugin already in the memory and the checkers already are registered, so they
> will be used.
>
> If you find this method hacky or not future proof, I could create an argument
> adjuster object here which could be used during the next steps. However
> StaticAnalyzerChecks have to be initialized early enough so that the user can
> list the checkers that are loaded from the plugin.
> Yes, the only effect of this function is to initialize the checker list.
Well, this contradicts to the explanation of how plugins are loaded that you
give below ;) But now I get it, and looks like the function should be called
"loadPlugins" or similar in order to make the important side-effect obvious
from the function name. And as we now rely on a mutable global state anyway, it
may be fine to introduce (#ifdefed only when plugins are enabled) an additional
global vector to store dynamically constructed static analyzer check list. At
least I don't see a substantially better alternative.
Filtering out -load arguments from the arguments we get from a compilation
database looks like a useful thing to do unless there's a better way to disable
loading of plugins during the analysis.
================
Comment at: clang-tidy/tool/ClangTidyMain.cpp:149
@@ -148,1 +148,3 @@
+static cl::opt<std::string> PluginPath(
+ "plugin-path",
----------------
xazax.hun wrote:
> alexfh wrote:
> > 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?
> I never tried to use multiple plugins, however you are right, the static
> analyzer supports to load multiple ones at the same time. I see your point to
> put this into ClangTidyOptions, so one can configure it using yaml files.
>
> My usecase is the following:
> We have a script, that uses LD_PRELOAD to load a shared object that hijacks
> the exec function call family and filters and logs compiler calls. After this
> log file is created a script invokes clang tidy with slightly modified
> compilation arguments. This way we can support any build system on posix
> systems including incremental build support.We just got the permission to
> open source this tool set, so it is possible that we will try to upstream
> this tool soon.
I'm still not sure about moving the "-plugin-path" option to `ClangTidyOptions`.
> My usecase is the following:
> We have a script, that uses LD_PRELOAD to load a shared object that
> hijacks the exec function call family and filters and logs compiler calls.
Similar to this one? https://github.com/rizsotto/Bear
> After this log file is created a script invokes clang tidy with slightly
> modified compilation arguments. This way we can support any build
> system on posix systems including incremental build support.
You mean you run analyses on each build? Sounds interesting, though there's a
(possibly more effective) alternative: to run analyses as a part of submitting
the code for review.
But anyway, that doesn't answer why do you need plugins and how you use them.
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
