Szelethus added a comment. Herald added subscribers: ASDenysPetrov, martong, steakhal.
> I have mixed feelings. Removing boilerplate is good, but the very fact that > we're legalizing this pattern indicates that our checkers will keep bloating > up, while i always wanted to actually split them instead (like, make > sub-checkers into their own separate //classes//, possibly spread out into > different files, kinda micro checkers as opposed to monolithic checkers (?)). The subchecker system as it works now is more diverse than that. Some systems use them purely for diagnostics (this patch is targeting those), while some affect the modeling as well. I think we should allow purely diagnostic checkers, because we can't really justify making an entire class for them, let alone an entire file, and they really are an integral part of the checker. However, we absolutely shouldn't promote adding further modeling into an existing checker whenever its avoidable. The unfortunate truth is (at least the way I see it) is that we can't really force anyone to write better code. I like to think this patch neither legalizes nor prevents someone from bloating a file further, but rather introduces a new tool to split the giant checkers up, or make future additions more manageable. The high level idea would be that when `CallDescriptionMap` is too simple, allow checkers to create their own events, and register subcheckers into them. This would for instance solve the problem of the unknown callback order of regular callbacks among checkers, which might be a better alternative then leaving this to `CheckerRegistry`. Btw I tried to write this comment for literally months now, but I admit that I don't yet the where such a subsystem could be deployed in the already existing checkers. I always think of `MallocChecker`, but I don't see how we could do it there just yet. @NoQ, in San José, you mentioned an example with `std::set` that would really demand a strong checker infrastructure, but I've since forgotten it. Could you explain it again please? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67336/new/ https://reviews.llvm.org/D67336 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
