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;
----------------
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 :)
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
