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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D66042#1625000 <https://reviews.llvm.org/D66042#1625000>, @Szelethus wrote:

> if we add this flag, people responsible for developing interafaces for the analyzer might end up using it.


And this is fine, that's supported. There's a very limited list of such people and we could talk to all of them easily if we wanted to. On the other hand, an end-user running `clang --analyze -Xclang -flagflagflag` manually on his desktop is not supported.

In D66042#1625000 <https://reviews.llvm.org/D66042#1625000>, @Szelethus wrote:

> Whenever we forget about adding a checker tag to a subchecker, this issue will arise again.


Mmm, if we forget about adding a checker tag to a subchecker, then we already have a problem, regardless of this patch, right? The checker name in the bug report is going to be incorrect in this case, which will, at the very least, hurt clang-tidy users.

In D66042#1625000 <https://reviews.llvm.org/D66042#1625000>, @Szelethus wrote:

> Your patch title, and the things things that you said make me believe that there are only a handful of core checkers that cause headaches, how about you add checker options to those instead? If the entirety of the core is problematic, you might as well add a package option to it.


My impression is kinda the opposite: the more we put our hacks into the imperative checker code, the harder it gets. `Checkers.td` "just works", but whenever we try to mess with it, we always get something wrong. Which is exactly why i greatly appreciated your work to formalize everything in tablegen: you allowed everybody to re-use the code that is easy to get wrong.

Generally, though, i think we're currently only talking about two checkers: the null dereference checker and the "calling a method on a null pointer" checker (which is, suddenly, a separate checker). @Charusso, do i understand correctly that we don't really want to disable uninitialized variable checkers (there are, like, 5 of them) because you've more or less fixed their false positives? Also we might want to disable MallocChecker but it doesn't really do any modeling and doesn't really belong to `core` either. If it's just two checkers, i definitely don't mind simply splitting them up for the purposes of GSoC, while letting us come to a consensus on this patch.

In D66042#1625000 <https://reviews.llvm.org/D66042#1625000>, @Szelethus wrote:

> If you still want to make a new functionality that could silence any checker, create an analyzer config. Those really are meant for development purposes only...


I have no data to conclude that frontend flags are more discoverable than `-analyzer-config` options. As a matter of our policy of what we actively support vs. what is passively available, those aren't supported.

In D66042#1625000 <https://reviews.llvm.org/D66042#1625000>, @Szelethus wrote:

> If this really is meant for scan-build only, you might as well get rid of the diagnostics at that level, without changing the analyzer. You said that you aren't really worried about the performance loss caused by bug report construction that will be suppressed later on, so it sounds ideal.


Hmm. This would require scan-build to parse HTML output, which is slightly annoying but not impossible, given that it already knows how to deduplicate reports. Tools that consume plist files should be fine because they anyway have to parse plist files. And clang-tidy already has their own silencing mechanism. So i kinda like this idea, even though it's a bit more work. @Charusso, could you take a look if it's actually too much work?


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

https://reviews.llvm.org/D66042