Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land.
In D64274#1623644 <https://reviews.llvm.org/D64274#1623644>, @NoQ wrote: > In D64274#1600834 <https://reviews.llvm.org/D64274#1600834>, @Szelethus wrote: > > > In D64274#1598226 <https://reviews.llvm.org/D64274#1598226>, @NoQ wrote: > > > > > 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? > > > Yes, generally i think it's good to have later options override earlier > options, because it allows people who run analysis through multiple layers of > scripts (eg., buildbots) to override their previous decisions without digging > through the whole pile of scripts. This isn't *that* important because the > use case is pretty rare, but if you have any immediate ideas on how to fix > this i'd be pretty happy :) The immediate idea is to never allow checkers that emit diagnostics to be dependencies. That way, we'll let the analyzer handle enabling/disabling them under the hood, and the checker dependency structure would be far more accurate, since no checker depends on diagnostics. What do you think? This seems to a hot topic anyways now, and would solve the issue behind core checkers as mentioned in D66042 <https://reviews.llvm.org/D66042>. I think it wouldn't be too much work, especially that I'm very familiar with this part of the codebase. That said, the solution might be lengthier than what folks would be comfortable merging so late into the release cycle. My entire checker dependency project was about getting rid of bugs to eventually list analyzer/checker options, but I think the idea of separating modeling/diagnostic parts should be a project-wide rule. I feel bad for dragging you through this, as I previously said In D64274#1572407 <https://reviews.llvm.org/D64274#1572407>, @Szelethus wrote: > [side note: since this checker doesn't really do any modeling, in fact it > wouldn even store a state if it inspected the call stack, it wouldn't be > logical to create a modeling checker that would be a dependency of both > `PureVirtualCall` and `VirtualCall`. Since `PureVirtualCall` is a strict > subset of `VirtualCall`, if it causes any crashes, you have to disable both > anyways. ] but now I believe having a common base would have been the ideal solution. Please feel free to commit as is, I'll pick this up as I enforce the rule. Maybe its also time to escalate this issue to cfe-dev. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64274/new/ https://reviews.llvm.org/D64274 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
