[PATCH] D84316: [analyzer][NFC] Split CStringChecker to modeling and reporting

Fri Jul 24 04:37:13 PDT 2020

Szelethus added a comment.

I personally preferred the previous diff, the reporting part of the checker and the string length modeling was separated far more cleanly.

In D84316#2171267 <https://reviews.llvm.org/D84316#2171267>, @NoQ wrote:

> Mmm, none of these benefits sound like they outweigh confusing the cost of users with a new checker flag that can't even be used in any sensible way.

I think the new checker would be an ideal candidate for a hidden checker, not to mention that D78126 <https://reviews.llvm.org/D78126>+D81761 <https://reviews.llvm.org/D81761> enforces it anyways with asserts. With that in mind, I don't see what we're giving up here.

> If you want separate files, just put the checker into a header and include it from multiple cpp files. A few checkers already do that - RetainCountChecker, MPIChecker, UninitializedObjectChecker. There's nothing fundamental about keeping checkers in an anonymous namespace. With that said, I don't want to make an example out of RetainCountChecker -- the way it is structured is not in line, at least the way I see it, with the modern way to develop large checkers.

I agree that there is no magical gain from keeping them there. UninitializedObjectChecker, btw, doesn't peek out -- only some of the related infrastructure.

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

https://reviews.llvm.org/D84316