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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
