NoQ added inline comments.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:483-504
+ if (!AnOpts.RawSilencedCheckersAndPackages.empty()) {
+ std::vector<StringRef> Checkers =
+ AnOpts.getRegisteredCheckers(/*IncludeExperimental=*/true);
+ std::vector<StringRef> Packages =
+ AnOpts.getRegisteredPackages(/*IncludeExperimental=*/true);
+
+ SmallVector<StringRef, 16> CheckersAndPackages;
----------------
Szelethus wrote:
> Szelethus wrote:
> > Charusso wrote:
> > > NoQ wrote:
> > > > Charusso wrote:
> > > > > Szelethus wrote:
> > > > > > Szelethus wrote:
> > > > > > > Charusso wrote:
> > > > > > > > Szelethus wrote:
> > > > > > > > > Szelethus wrote:
> > > > > > > > > > Szelethus wrote:
> > > > > > > > > > > The reason why I suggested validating this in
> > > > > > > > > > > CheckerRegistry is that CheckerRegistry is the only class
> > > > > > > > > > > knowing the actual list of checkers and packages, and is
> > > > > > > > > > > able to emit diagnostics before the analysis starts. This
> > > > > > > > > > > solution wouldn't work with plugin checkers/packages.
> > > > > > > > > > I don't see this being addressed actually?
> > > > > > > > > >
> > > > > > > > > > I think it would be totally fine to just omit the
> > > > > > > > > > validation part as I said earlier, the patch will be
> > > > > > > > > > leaner, and cases in which we're using the silencing of
> > > > > > > > > > checkers are very exotic anyways.
> > > > > > > > > Also, we should probably compliment such validation by
> > > > > > > > > actually writing tests for plugins.
> > > > > > > > >
> > > > > > > > > I've been through that process once. It isn't fun.
> > > > > > > > > Really-really isn't :^) Let's just collectively agree to
> > > > > > > > > "forget" this :)
> > > > > > > > Checker validation is in `CheckerRegistry`, configuration
> > > > > > > > validation is in `parseAnalyzerConfigs()`. I have made a
> > > > > > > > configuration, rather than a checker flag, so that I have not
> > > > > > > > found more appropriate place and its does the job well. If it
> > > > > > > > will be needed externally, I hope someone could do better.
> > > > > > > Well isn't this checker validation?
> > > > > > In any case, could just throw in a FIXME before commiting please? :)
> > > > > @NoQ, does the `AnalyzerOptions::getRegisteredCheckers()` contain the
> > > > > plugins?
> > > > //*/me doesn't know much about plugins*//
> > > >
> > > > Normally enable-disable flags do work on plugins, so plugin checkers
> > > > must make it into the registry at some point.
> > > So do we need a FIXME or most likely it working with plugins?
> > Which doesn't happen here. CheckerRegistry is the only class supplied with
> > this information.
> >
> > A primitive way to demonstrate this is the following:
> > `AnalyzerOptions::getRegisteredCheckers()` is a static function without
> > parameters that doesn't use global variables, so it's a hair away from
> > being `constexpr` as well. Plugins are an inherently runtime thing. A more
> > concrete proof is that it only works with the inclusion of `Checkers.inc`,
> > which is generated compile time by TableGen.
> >
> > This was the primary reason behind us struggling really hard with checker
> > dependencies and checker options, because we can't solve this problem with
> > the use of TableGen only (relieving us of any runtime overhead). This is
> > also the main reason behind my caution whenever this part of the project is
> > touched -- we're in an okay spot now compared to when these systems got
> > implemented, but we're quite a distance away from perfect.
> >
> > You can read more about plugins and checker registration here, that I'll
> > promise myself to finish after GSoC is wrapped up: D58065
> I know for a fact that this wouldn't work with plugins, believe me :)
Anyway, it's either "FIXME: This doesn't work with plugins" or "TODO: Does this
work with plugins?".
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66042/new/
https://reviews.llvm.org/D66042
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
