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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
