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

[Balázs Benics via Phabricator via cfe-commits]

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



================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringChecker.cpp:32-34
+auto CStringChecker::createOutOfBoundErrorMsg(StringRef FunctionDescription,
+                                              AccessKind Access)
+    -> ErrorMessage {
----------------
NoQ wrote:
> Why suddenly use arrow syntax here?
This way I could spare the full name of the class :D
I will use the qualified name instead.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringChecker.h:227
+#endif
\ No newline at end of file

----------------
NoQ wrote:
> No NeWlInE aT eNd Of FiLe
I'm wondering why did clang-format not add this - I'm really surprised. Thanks.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLengthModeling.cpp:309
+
+// TODO: Is it useful?
+void CStringChecker::printState(raw_ostream &Out, ProgramStateRef State,
----------------
NoQ wrote:
> Yes it is. It gets invoked during exploded graph dumps and it's an invaluable debugging facility.
A strange observation to note here.
In the implementation of the dump method, I use the provided  `NL` and `Sep` parameters.
However, in the `checker_messages` of a State dump, `Sep` seem to be substituted with the empty string.
For example [[ https://github.com/llvm/llvm-project/blob/86e1b73507f3738f10eefb580d7c5e9adf17c6c0/clang/lib/StaticAnalyzer/Checkers/Taint.cpp#L37 | taint::printTaint ]] just ignore the `Sep` parameter to //possibly// workaround this issue.


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

https://reviews.llvm.org/D84316