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

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

Szelethus added a comment.

In D75271#1896182 <https://reviews.llvm.org/D75271#1896182>, @Szelethus wrote:

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


Thinking back, I did have a number of failed attempts to make something a bit less ugly, but the sharp divide between the 2 libraries makes is really-really difficult, and I don't recall alternative solutions being that much better. Either the checker interface gets worse, or the checker registration interface gets so messy that it would severely hurt further improvements in terms of checker dependency development.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75271