[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

There is a discussion to be had about the core package. So @NoQ suggested that we could hide the entire thing just as we do with apiModeling, but I argued that the users wouldn't be exposed to the checker descriptions. However, it makes little sense to caution our users not to disable it (because things might break), and at the same time giving them the option. If a core checker emits dumb messages, of course I'd like get rid of them (this entire patch is about that, isn't it?). I think it would make a lot more sense to create a separate (and hidden!) coreModeling package that would do all the modeling, and regard core as a highly recommended, but not mandatory set of checkers. Wouldn't this create a cleaner interface?

All in all, I sympathize with this problem, but admit that I don't particularly like the silencing approach, because we try hard to hide implementation details from users, and this forces them in a way to interact with it. I realize however that there isn't much time left of GSoC, and separating who knows how many checkers would take a long time, so if this is important, I don't insist. We could however place a couple FIXMEs indicating that this is a placeholder solution, and not advertise this flag too much.

Please know that this is a complaint on core checkers, not really your patch, and I appreciate your work in finally making sense of this! :)



================
Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:192-197
+  /// Pair of checker name and enable/disable to do analysis.
+  std::vector<std::pair<std::string, bool>> CheckerAnalysisVector;
+
+  /// Vector of checker names to do not emit warnings.
+  std::vector<std::string> CheckerSilenceVector;
 
----------------
Charusso wrote:
> NoQ wrote:
> > `CheckerAnalysisVector` sounds like a vector of checkers that will be subject to analysis. But in reality they are the ones that are analyzing and nobody is analyzing them.
> > 
> > The old name isn't very expressive either, but at least it sounds cool >.<
> > 
> > Maybe `EnabledCheckers`, `SilencedCheckers`?
> The problem with `EnabledCheckers` it is a lie. It would be just `Checkers` and then whether a given checker is enabled or disabled is determined later. `CheckerEnableVector` and `CheckerSilenceVector` may would be okay.
Aye, I've come across this field multiple times and there isn't really a good name for it. However, is this correct? Are these *checkers*, or checkers *and* packages?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66042/new/

https://reviews.llvm.org/D66042