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

steakhal added a comment. I would like to discuss why don't we have a distinct checker managing the bookkeeping stuff of the CString lengths. I just want a clean understanding and wide consensus about this. Personally, I would still prefer the original version of this patch (//+nits of course//

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

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringChecker.cpp:32-34 +auto CStringChecker::createOutOfBoundErrorMsg(StringRef FunctionDescription, + AccessKind Access) +-> ErrorMessage {

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

steakhal added a comment. In D84316#2233368 , @Szelethus wrote: > Do I sense correctly that the only information `CSrtingLengthModeling.cpp` > requires from the actual `CStringChecker` is a checker tag? AFAIK yes. > [...] it seems like we're legalizing

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

Szelethus added a comment. Do I sense correctly that the only information `CSrtingLengthModeling.cpp` requires from the actual `CStringChecker` is a checker tag? Because if so, I think we should just separate them even more cleanly -- we could just make a `CStringLengthModeling` checker impleme

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

steakhal added a comment. Ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84316/new/ https://reviews.llvm.org/D84316 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

steakhal added a comment. In D84316#2187372 , @19n07u5 wrote: > The title is a little bit confusing because only the C-string size model is > going to be separated and be accessible. Could you elaborate on why is the title not precise? It seems that the

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

19n07u5 added a comment. The title is a little bit confusing because only the C-string size model is going to be separated and be accessible. Other than that as @NoQ pointed out we need lot more of these common-API-separation patches. It is a great starting point for the `CStringChecker`. ==

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

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLength.h:43 + ProgramStateRef &State, const Expr *Ex, + SVal Buf, bool Hypothetical = false); +

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

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLength.h:43 + ProgramStateRef &State, const Expr *Ex, + SVal Buf, bool Hypothetical = false); +

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

steakhal added a comment. Thanks for checking this @balazske. Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLength.h:43 + ProgramStateRef &State, const Expr *Ex, + SVal Buf, bool Hypot

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

balazske added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLength.h:43 + ProgramStateRef &State, const Expr *Ex, + SVal Buf, bool Hypothetical = false); +

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

Szelethus added a comment. In D84316#2171270 , @NoQ wrote: > Imagine something like re-using the state trait implementation between > `MallocChecker` and `StreamChecker` because they both model "resources that > can be deallocated twice or leaked" - rega

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

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 , @NoQ wrote: > Mmm, none of these benefits sound like they outweigh c

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

steakhal updated this revision to Diff 280398. steakhal added a comment. - Created the `CStringChecker` subdirectory holding the related files. - `CStringChecker` split up to 4 files: - `CStringChecker.h`: Declaration of the checker class. - `CStringChecker.cpp`: Definitions ONLY of the cstirn

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

NoQ added a comment. In D84316#2168730 , @steakhal wrote: > In D84316#2168462 , @NoQ wrote: > > > Such separation also looks amazing because ultimately the first part can be > > made checker-inspecific (i.e., a reu

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

NoQ added a comment. In D84316#2168730 , @steakhal wrote: > I wanted to have a separate class for bookkeeping while minimalizing the > necessary changes. > What do you think would be the best way to organize this separation? > > Few notes: > > - Checkers

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

Szelethus added a comment. You could do it in the code, but if the modeling wouldn't be present from CStringModeling, CStringChecker wouldn't work properly. So you should make it a strong dependency, just as you did in this patch. My comment was mainly a response to @NoQ :)

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

steakhal marked 4 inline comments as done. steakhal added a comment. In D84316#2169195 , @Szelethus wrote: > [...] you really cant make CStringChecker work without CStringModeling How should I fuse the `CStringModeling` and the `CStringChecker` together

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

Szelethus marked an inline comment as done. Szelethus added a comment. Yay! This checker has become a major headache as the analyzer grew. Not on a RetainCount scale, but on a MallocChecker one. Cheap shots, I know :) The patch looks great! I have some miscellaneous comments, but nothing blockin

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

steakhal added a comment. In D84316#2168462 , @NoQ wrote: > Shared accessors look amazing. > > [...] you're splitting up the part which performs boring bookkeeping for the > state trait from the part which models `strlen()` and other various functions.

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

NoQ added a comment. Shared accessors look amazing. If i understood correctly, you're splitting up the part which performs boring bookkeeping for the state trait from the part which models `strlen()` and other various functions. Such separation also looks amazing because ultimately the first p