[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Szelethus added a comment. In D66042#1629019 , @alexfh wrote: > In D66042#1627760 , @NoQ wrote: > > > In D66042#1627193 , @alexfh wrote: > > > > > But without this patch

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

alexfh added a comment. In D66042#1627760 , @NoQ wrote: > In D66042#1627193 , @alexfh wrote: > > > But without this patch clang seems to have the same two ANALYZE log lines > > regardless of whether I enable

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

NoQ added a comment. I'm convinced, let's keep everything as is but turn this into an `-analyzer-config` flag. We generally wanted to reduce the number of frontend flags because they are more taxing on backwards compatibility, so it makes perfect sense. CHANGES SINCE LAST ACTION

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Szelethus added a comment. In D66042#1627842 , @Charusso wrote: > Any analyzer config flag is equally accessible to anyone as the driver flags > as they are both flags. The only difference is the config flags are more code > to implement, and a lot more

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

NoQ added a comment. In D66042#1627842 , @Charusso wrote: > @Szelethus, here is a really cool example: > https://clang.llvm.org/docs/ClangCommandLineReference.html. These are driver flags. They are indeed well-documented and user-facing. Frontend

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Charusso added a comment. Any analyzer config flag is equally accessible to anyone as the driver flags as they are both flags. The only difference is the config flags are more code to implement, and a lot more difficult to use. @NoQ, why the hell would we pick another type of flag which makes

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

NoQ added a comment. In D66042#162 , @Szelethus wrote: > Yup. We will be able to present the drastic improvement made on LLVM > analyses, while giving us some breathing room to polish a long-term solution. > I suspect `-analyzer-config

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Szelethus added a comment. In D66042#1627765 , @NoQ wrote: > In D66042#1626631 , @Szelethus wrote: > > > In D66042#1626513 , @Charusso > > wrote: > > > > > I really

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

NoQ added a comment. In D66042#1626631 , @Szelethus wrote: > In D66042#1626513 , @Charusso wrote: > > > I really appreacite your ideas. It is unbelievable you guys bring up 20 > > different ideas for 5 LOC. I

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

NoQ added a comment. In D66042#1627193 , @alexfh wrote: > Should this be different with clang and this patch? Nope. Moreover, this patch in fact introduces the same problem in scan-build :) In D66042#1627193 ,

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

alexfh added a comment. In D66042#1625898 , @NoQ wrote: > While we're here: i poked your silencing mechanism a little bit and even > though i'm still pretty sure you couldn't have done it perfectly without our > help, it sounds as if the only problem

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Szelethus added a comment. Too bad we're in different time zones, I can only respond in a giant comment, but I wouldn't want to make anyone feel left out :) The conclusion to my responses is this: - I recently sent out a mail to cfe-dev (I hope to have added your correct email adresses!) that

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Charusso added a comment. In D66042#1626460 , @NoQ wrote: > I'd like to hear @Szelethus's opinion one more time on this patch. I'm > perfectly fine with abandoning the idea and going for in-checker > suppressions, simply because there's at least one

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

NoQ added a comment. I'd like to hear @Szelethus's opinion one more time on this patch. I'm perfectly fine with abandoning the idea and going for in-checker suppressions, simply because there's at least one strong opinion against it and i don't want to push this further, but i just honestly

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Charusso added a comment. In D66042#1625971 , @NoQ wrote: > In D66042#1625000 , @Szelethus wrote: > > > if we add this flag, people responsible for developing interafaces for the > > analyzer might end up using

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Charusso added a comment. In D66042#1626122 , @NoQ wrote: > > use it locally > > What do you mean here? If you want to use the patch for evaluating your > results, you might as well untick the checker in the scan-build's index.html > interface. The

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

NoQ added a comment. > use it locally What do you mean here? If you want to use the patch for evaluating your results, you might as well untick the checker in the scan-build's index.html interface. The point of having this patch landed is to allow users who are worried by false positives of

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Charusso added a comment. In D66042#1625971 , @NoQ wrote: > In D66042#1625000 , @Szelethus wrote: > > > Your patch title, and the things things that you said make me believe that > > there are only a handful of

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

NoQ added a comment. In D66042#1625000 , @Szelethus wrote: > if we add this flag, people responsible for developing interafaces for the > analyzer might end up using it. And this is fine, that's supported. There's a very limited list of such people

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

NoQ added a comment. In D66042#1625235 , @alexfh wrote: > In D66042#1624081 , @NoQ wrote: > > > +@alexfh because clang-tidy now finally has a way of safely disabling core > > checkers without causing crashes all

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

alexfh added a comment. In D66042#1624081 , @NoQ wrote: > +@alexfh because clang-tidy now finally has a way of safely disabling core > checkers without causing crashes all over the place! Would you like to take > the same approach as we picked in

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Szelethus added a comment. In D66042#1624697 , @Charusso wrote: > In D66042#1624684 , @NoQ wrote: > > > My idea here was that this new feature isn't going to be user-facing. We > > aren't promising to support all

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

NoQ added a comment. In D66042#1624697 , @Charusso wrote: > `StringRef("osx.cocoa.RetainCountBase").startswith("osx.cocoa.RetainCount")` > is true, so there is no real issue until we manage the prefixes well. I > assume that the user who knows how to

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Charusso added a comment. In D66042#1624684 , @NoQ wrote: > My idea here was that this new feature isn't going to be user-facing. We > aren't promising to support all combinations of > enabled-disabled-silenced-dependent-registercheckerhacks, but

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

NoQ added a comment. My idea here was that this new feature isn't going to be user-facing. We aren't promising to support all combinations of enabled-disabled-silenced-dependent-registercheckerhacks, but instead use the new feature when we know it'll do exactly what we want. It is going to be

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Szelethus requested changes to this revision. Szelethus added a comment. This revision now requires changes to proceed. In D66042#1624478 , @Charusso wrote: > In D66042#1624475 , @Szelethus wrote: > > > In

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Charusso added a comment. In D66042#1624475 , @Szelethus wrote: > In D66042#1624469 , @Charusso wrote: > > > In a long-term rewriting the Analyzer from scratch would be ideal. There is > > no problem with this

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Szelethus added a comment. In D66042#1624469 , @Charusso wrote: > In D66042#1624468 , @Szelethus wrote: > > > Speaking about performance impact, note where your patch does the actual > > silencing: by the time

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Szelethus added a comment. In D66042#1624468 , @Szelethus wrote: > 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

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Charusso added a comment. In D66042#1624468 , @Szelethus wrote: > In D66042#1624462 , @Charusso wrote: > > > My solution preserve the checkers as they are and yours definitely would > > rewrite them. Checker

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Szelethus added a comment. In 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

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Charusso added a comment. In D66042#1624432 , @Szelethus wrote: > In D66042#1624365 , @Charusso wrote: > > > In D66042#1624320 , @Szelethus > > wrote: > > > > > I think it

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Szelethus added a comment. In D66042#1624365 , @Charusso wrote: > In D66042#1624320 , @Szelethus wrote: > > > I think it would make a lot more sense to create a separate (and hidden!) > > coreModeling package that

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Charusso added a comment. In D66042#1624320 , @Szelethus wrote: > I think it would make a lot more sense to create a separate (and hidden!) > coreModeling package that would do all the modeling, and regard core as a > highly recommended, but not

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Szelethus added a comment. There is a discussion to be had about the core package. So @NoQ suggested that we could hide the entire thing just as we do with apiModeling, but I argued that the users wouldn't be exposed to the checker descriptions. However, it makes little sense to caution our

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Charusso added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:192-197 + /// Pair of checker name and enable/disable to do analysis. + std::vector> CheckerAnalysisVector; + + /// Vector of checker names to do not emit warnings. +

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. But i definitely like it how smooth this patch turned out to be! Also recent bugzilla requests for this feature: https://bugs.llvm.org/show_bug.cgi?id=42816

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

NoQ added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:192-197 + /// Pair of checker name and enable/disable to do analysis. + std::vector> CheckerAnalysisVector; + + /// Vector of checker names to do not emit warnings. + std::vector

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Charusso updated this revision to Diff 214500. Charusso added a comment. - Fix a comment. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66042/new/ https://reviews.llvm.org/D66042 Files: clang-tools-extra/clang-tidy/ClangTidy.cpp clang/include/clang/Driver/CC1Options.td

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Charusso updated this revision to Diff 214498. Charusso marked 5 inline comments as done. Charusso added a comment. - Fix. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66042/new/ https://reviews.llvm.org/D66042 Files: clang-tools-extra/clang-tidy/ClangTidy.cpp

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Charusso added inline comments. Comment at: clang/include/clang/Driver/CC1Options.td:127-128 +def analyzer_disable_warning : Separate<["-"], "analyzer-disable-warning">, + HelpText<"Choose analyzer checkers of the warnings to disable">; +def analyzer_disable_warning_EQ :

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

NoQ added reviewers: alexfh, Szelethus. NoQ added a subscriber: alexfh. NoQ added a comment. +@alexfh because clang-tidy now finally has a way of safely disabling core checkers without causing crashes all over the place! Would you like to take the same approach as we picked in scan-build, i.e.

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Charusso updated this revision to Diff 214490. Charusso added a comment. - Remove one misleading 'no-warning' comment. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66042/new/ https://reviews.llvm.org/D66042 Files: clang-tools-extra/clang-tidy/ClangTidy.cpp

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

Charusso created this revision. Charusso added a reviewer: NoQ. Charusso added a project: clang. Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun. This patch introduces a new option: