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