[PATCH] D64274: [analyzer] VirtualCallChecker overhaul.

Thu Jul 25 04:52:28 PDT 2019

Szelethus requested changes to this revision.
Szelethus added a comment.
This revision now requires changes to proceed.

If either checker emits an error, the current implementation would say it originates from `cplusplus.PureVirtualCall`. Could you please create a new `ProgramPointTag` with the correct checker name for `optin.cplusplus.VirtualCall`? You may retrieve that name through `CheckerManager::getCurrentCheckName()`.

In D64274#1598226 <https://reviews.llvm.org/D64274#1598226>, @NoQ wrote:

> In D64274#1586282 <https://reviews.llvm.org/D64274#1586282>, @Szelethus wrote:
>
> > //Ackchyually//,  it doesn't per se break anything, but will result in CodeChecker no longer enabling `optin.cplusplus.VirtualCall` :^) Sorry, oversight on my end. Observe the following monster of a clang invocation...
>
>
> Mmm. Aha. Ok, i can reproduce your problem, but i don't think reversing the dependencies is gonna work. If we make the pure checker depend on the optin checker, we would always have the opt-in checker registered first, and therefore there's no way to figure out if we really wanted to opt in into the optin checker or we are simply loading it as a dependency. Which, in particular, makes it impossible to discriminate between `-analyzer-checker cplusplus.PureVirtualCall` and `-analyzer-checker optin.cplusplus.VirtualCall` when the option is unset: in both cases we'll first register the opt-in checker and then the pure checker.

I'm sold. Let's drop this issue and we'll make sense of the CodeChecker runline by the time Clang 10.0.0 drops.

> I'm confused though; the way i was previously understanding your work, when disabling a dependency and then enabling a dependent checker *later* in the run-line, it should re-enable the dependency automatically. Did you consciously decide not to implement it that way?

Yes, somewhat. I chose another direction, which is simply not exposing base checkers (which I detailed in D60925 <https://reviews.llvm.org/D60925>) to the users (`-analyzer-checker-help-developer` is a thing now), so they aren't ever enabled if none of their dependent checkers are. This worked wonders, because, interestingly, none of the base checkers were emitting diagnostics.

I find this clearer, if I disable something because it crashes on my code, I don't want to see it again. Though, 9.0.0-final isn't out, and I'm sorry that I didn't make this decision clear enough -- shall I fix it?

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

https://reviews.llvm.org/D64274