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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D66042#1625971 <https://reviews.llvm.org/D66042#1625971>, @NoQ wrote:

> 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.


We cannot be sure at the end of GSoC which checkers are needed to be silenced. For example I am about to model the casting enough well, so that every path we take should be meaningful according to the dynamic casting in LLVM.

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

Because clang-tidy has its own feature for disabling the core (as just revealed) it would be possible to only affect scan-build with that patch. However touching that Perl-madness to read information and remove reports would require too much overhead compared to a comfy way to apply that existing patch locally. `sub ScanFile` in scan-build is already does that logic, so it would not hurt too much, just I do not like the idea touch that tool. I think not just scan-build would use that feature I have just implemented, as this is the only comfortable way to disable the core. That is why it is inside the Analyzer, and I believe every other place to implement would be a bad idea, as everyone really needs that feature. I think if we have strong problems with a patch, it meaningless to continue/rethink from 10 different angles and apply it locally for the given task is far more better solution. Also I cannot really force out to implement that new feature in every scan-build like tooling, and since then this patch would bring up overhead without actual implementations.

In my mind every new API/feature like the NoteTag or the CallDescriptionMap should arrive in the code base with removing the old API. So we will have better code and no-one would use the old API and instead would improve the new much better API without continuing to create more and more deprecated code (that is happening with NoteTags ATM). Because we are working with tons of experimental features, like that two new improvement, we do not have such policy to require to deprecate the old API. As we have no policies according to experimental features and that patch is an experimental feature, I see two directions: publish that as it is, or abandon it, and use it locally.


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

https://reviews.llvm.org/D66042