Szelethus added a comment. In D66042#1624462 <https://reviews.llvm.org/D66042#1624462>, @Charusso wrote:
> My solution preserve the checkers as they are and yours definitely would > rewrite them. Checker writing has tons of boilerplates, I think adding more > should be avoided. Why would you touch the checkers as my solution is already > implemented and working perfectly without any overhead? Speaking about performance impact, note where your patch does the actual silencing: by the time control reaches that point, we created bug report equivalence classes, constructed a trimmed version of the exploded graph, constructed a bug path from that trimmed graph and ran all visitors on and have gathered all diagnostic pieces for it (plenty of shared object creations there). I have a strong suspicion that not even creating the bug report is far faster. I think that adding yet another way of controlling checkers is confusing, and solves a problem that shouldn't exist in the first place. Now, that being said, the problem //does// exist, and I see this as an elegant solution for the time being. >>> Also this patch aims to hide 600 `cast<>` related reports, and try to fix >>> that ambiguity to "disable a core checker" as we really mean that to do not >>> emit uninteresting reports. Personally I am really against the idea to make >>> the core modeling disable-able, this scan-build addition fix that, you >>> cannot disable it. If crash occurs with the core package then we should >>> give it a 9000 priority level on Bugzilla and `fix-it`. >> >> My wording may have been poor, apologies for the misunderstanding -- of >> course I do not intend to get rid of the modeling, just achieving the same >> goal from a different angle. `coreModeling` would be a dependency of all >> path sensitive checkers (we did talk about this in one of my patches), and >> the user would no longer be exposed to the option of disabling them, only >> their diagnostics. This is similar to the way how the checker dependency >> system was reimplemented as well, and I like to think that the analyzer's >> interface really benefited from it. >> >> What do you think? > > At the moment we have a way to disable core modeling because they could break > the user's analysis. My patch only touch the scan-build's invocation as an > **experimental** feature to make it impossible to disable the core modeling > **as a side effect** and **only trough scan-build**. Mainly the idea was to > use the fewest possible commands using scan-build therefore now one > `-disable-checker` command does two things. I hope that it is useful for > other users as well to "disable" the core checkers. > > However, if you have time to continue your dependencies patch to make the > core modeling non-disable-able with making the core checkers bulletproof, > that would be neat! But that could be some other patch and out of the scope > of that patch. Most definitely! Would you agree that in the long term such a division would be a more ideal approach? Because if we're commiting ourselves to silencing checkers, there may be some annoying things to fix too, like, if a checker emits a warning with the incorrect name (this is unfortunately not too uncommon, for example when the checker tag is associated with the modeling checker), the user would be confused why the silence didn't go through, or silenced far too many things. If this is just a band aid solution only for scan-build, only for core checkers, wouldn't an analyzer-config be better? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66042/new/ https://reviews.llvm.org/D66042 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
