Charusso added a comment.
Thanks for the reviews!
================
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;
----------------
NoQ wrote:
> 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?".
Okay, cool, thanks! I like the `FIXME` category:
```
// FIXME: Here we try to validate the silenced checkers or packages are valid.
// The current approach only validates the registered checkers which does not
// contain the runtime enabled checkers and optimally we would validate both.
```
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
