Szelethus 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;
----------------
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
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66042/new/
https://reviews.llvm.org/D66042
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
