Charusso 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;
----------------
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?


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

Reply via email to