Charusso added a comment. In D66042#1624468 <https://reviews.llvm.org/D66042#1624468>, @Szelethus wrote:
> 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. Let us say an average user using scan-build add that flag: `-disable-checker core.DivideZero`. I am 100% sure the user meant that to disable that checker's reports. Whatever it takes, the main modeling and main graph-building is what burns the companies money on electricity bills. We have tons of ways to suppress reports, because it makes zero overhead in cost. >>>> 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? In a long-term rewriting the Analyzer from scratch would be ideal. There is no problem with this patch, it will not cause any issues like that. May I would like to disable the apiModeling as well, with my patch it is one command. With your approach it would require to rewrite all of the existing apiModeling checkers after the core ones for no reason as we have better way: this patch. 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