[PATCH] D75271: [Analyzer][NFC] Add AnalyzerOptions parameter to shouldRegisterXXX() functions

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a reviewer: Charusso.
Szelethus added a comment.

In D75271#1895900 <https://reviews.llvm.org/D75271#1895900>, @Charusso wrote:

> I am so sorry to mention, but we need the `AnalysisManager` to pass around which manages the analysis, therefore it knows both the `LangOptions` and `AnalyzerOptions`.




In D75271#1895915 <https://reviews.llvm.org/D75271#1895915>, @Charusso wrote:

> PS: The `CheckerManager` also could serve this behavior as `registerXXX()` already passing around that manager, but I believe the `AnalysisManager` supposed to manage the analysis.


Well, I like to say that "Any manager can be retrieved in clang at arbitrary places if you try hard enough", so I think either that //actually// satisfies this would be fine :)

> Also this entire callback should be removed ideally: it has to be a virtual function defaulting to `return true;` and if someone needs this feature could rewrite the behavior. I guess there was some debate whether it should be on by default or not, but for a checker writer and future changes this patch shows that how weak this API is.

Yup, that is a very good criticism of the the API. I would prefer to see something a lot less ugly, and I strongly share you sentiment that making changes to it is about as ugly as the initial patch itself. Virtual functions would be okay, the reason why I didn't do that is because where do you put them? Checker objects can't house them, because the point of the entire `shouldRegister` function is to never create them in the first place. But thinking about it again, the problem isn't really their **creation** as much as their **registration**. If we were to create a checker object and ask it whether its fine to do analysis, and //then// store it, we would be able to get rid of the `shouldRegister` function. I can see from a mile away that due to the library layout, cyclic linking dependencies, and whatever else would make this seemingly trivial task a lot more invasive and difficult, and I'm just not too sure whether its worth the effort to change an arguably bearable inconvenience.

In D75271#1896098 <https://reviews.llvm.org/D75271#1896098>, @Charusso wrote:

> In D75271#1895949 <https://reviews.llvm.org/D75271#1895949>, @baloghadamsoftware wrote:
>
> > It is impossible to use `CheckerManager` as parameter for `shouldRegisterXXX()` because `CheckerRegistry` does not have it. If I add `CheckerManager` to `CheckerRegistry` then the `printXXX()` functions will not work because they do not have `CheckerManager` at all. This patch does not help in printing error message, see D75171 <https://reviews.llvm.org/D75171>. I need a way to solve 44998 <https://bugs.llvm.org/show_bug.cgi?id=44998> by rejecting the checker if the option is disabled **and** printing an error message.
>
>
> Aha, I see as of D75171 <https://reviews.llvm.org/D75171>. Well, @Szelethus felt the same to pass around the `CheckerManager`. Let us see:
>
>   std::unique_ptr<CheckerManager> ento::createCheckerManager(
>       ASTContext &context,                                     
>       AnalyzerOptions &opts,
>       ArrayRef<std::string> plugins,
>       ArrayRef<std::function<void(CheckerRegistry &)>> checkerRegistrationFns,
>       DiagnosticsEngine &diags) {                                               
>     auto checkerMgr = std::make_unique<CheckerManager>(context, opts);
>                                                                        
>     CheckerRegistry allCheckers(plugins, diags, opts, context.getLangOpts(),
>                                 checkerRegistrationFns);                      
>                                                          
>     allCheckers.initializeManager(*checkerMgr);
>     allCheckers.validateCheckerOptions();        
>     checkerMgr->finishedCheckerRegistration();
>                                                
>     return checkerMgr;
>   }
>
>
> - from `StaticAnalyzer/Frontend/CheckerRegistration.cpp`.
>
>   Are you sure we cannot pass around the manager like `allCheckers.initializeManager(*checkerMgr);`? I am also thinking of a glue-layer between the two managers, like `CheckerRegistryManager` or something, which holds all your needs, like `AnalyzerOptions` and `LangOptions` for now.


`CheckerManager` has complete control of `CheckerRegistry`. You can for sure cascade it down as a parameter. The worry is, again, how `CheckerManager` is in `libStaticAnalysisCore` and `CheckerRegistry` is in `libStaticAnalysisFrontend`, but given that we wouldn't need to use it one but more then how we use it for registry functions, this should be fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75271