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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D66042#1624432 <https://reviews.llvm.org/D66042#1624432>, @Szelethus wrote:

> In D66042#1624365 <https://reviews.llvm.org/D66042#1624365>, @Charusso wrote:
>
> > In D66042#1624320 <https://reviews.llvm.org/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 mandatory set of checkers. Wouldn't this create a cleaner interface?
> >
> >
> > Sadly separating a modeling package is impossible. The checkers subscription-based design is made for that purpose to make them both model the analysis, and both emit reports. None of the checkers has separated modeling and separated business logic, because they are so tied together. The reporting part fires on given modeling parts, like that is why they could report errors during modeling and stop the modeling. May if we redesign all the core checkers to separated business/modeling logic it would be helpful.
>
>
> I didn't mean to do too invasive changes, only something like this, which would be tedious but not difficult:
>
>   struct ModelingChecker {
>     bool AreDiagnosticsEnabled = false;
>     void model() const;
>   };
>  
>   void ModelingChecker::model() const {
>     // whatever
>  
>     if (!AreDiagnosticsEnabled) {
>       C.generateSink(State, C.getPredecessor());
>       return;
>     }
>  
>     static CheckerProgramPointTag DiagnosticCheckerTag(this, "DiagnosticChecker");
>     const ExplodedNode *ErrorNode = C.generateErrorNode(State, &DiagnosticCheckerTag);
>     // etc etc
>   }
>  
>   void registerDiagnosticChecker(CheckerManager &Mgr) {
>     Mgr.getChecker<ModelingChecker>()->AreDiagnosticsEnabled = true;
>   }
>
>
> A solution like this would preserve the current checker structures while neatly hiding the implementation part.


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?

>> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66042/new/

https://reviews.llvm.org/D66042