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