[PATCH] D54438: [analyzer] Reimplement dependencies between checkers

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

Szelethus added a comment.

In D54438#1372324 <https://reviews.llvm.org/D54438#1372324>, @NoQ wrote:

> *gets hyped for the upcoming patchlanding party*


Oh yeah, super excited about this! It was a blast!

> In D54438#1329425 <https://reviews.llvm.org/D54438#1329425>, @Szelethus wrote:
> 
>> Hmmm, I left plenty of room for improvement in the tblgen generation, we could generate compile-time errors on cycles within the dependency graph, try to include the generated file only once, but these clearly are very low-prio issues, so I'll do it later. I'll have to revisit the entire thing soon enough.
> 
> 
> Hmm. The dependency cycle check sounds cool as long as we do actually have problems with dependency cycles. I guess we could just enable/disable the whole cycle of checkers all together? This might even be a useful feature.

That sounds pretty cool actually! For all the crap I give people about documentation, I'm struggling quite a bit with this one (will probably end up making one for the entirety of `Frontend/`), but will definitely include this in there.

> What do you mean by "include the generated file only once"?

Never mind. It was thinking aloud, which I later realized is nonsense.

> Aha, ok, so the current approach to conflict resolution is to only enable the checker if none of its dependencies were disabled on the command line. So if, say, `DependentChecker` depends on `BaseChecker`, once an -analyzer-disable-checker `BaseChecker` flag is passed, it needs to be reverted via `-analyzer-checker BaseChecker` before the `-analyzer-checker DependentChecker` directive would take effect, regardless of in which order it is with respect to the `BaseChecker`-enabling/disabling directives.

Exactly.

> So we kinda choose to err on the side of disabling in ambiguous situations. Which kinda makes sense because the disable-argument is more rare and indicates an irritation of the user. But it is also kinda inconsistent with how the options interact in absence of dependencies: "the flag that was passed last overrides all previous flags". And we can kinda fix this inconsistency by introducing a different behavior:
> 
> - whenever an `-analyzer-checker` flag is passed, remove the "disabled" marks from all checkers it depends on;
> - whenever an `-analyzer-disable-checker` flag is passed, remove the "enabled" marks from all checkers that depend on it.
> 
> This approach still ensures that the set of enabled checkers is consistent (i.e., users are still not allowed to shoot themselves in the foot by running a checker without its dependencies), but it also looks respects every flag in an intuitive manner. WDYT?

Sounds great! Got nothing against that.

Hmm, I didn't really add tests about the dependency related potential implicit disabling, not even a warning, so there still is more work to be done.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54438