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

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

steakhal marked 4 inline comments as done.
steakhal added a comment.

In D84316#2169195 <https://reviews.llvm.org/D84316#2169195>, @Szelethus wrote:

> [...] you really cant make CStringChecker work without CStringModeling


How should I fuse the `CStringModeling` and the `CStringChecker` together while splitting them up?

I mean, that would be the best if the `CStringChecker` would focus on modeling the related cstring functions while letting the `CStringModeling` do the bookkeeping.
I see some contradiction here.



================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:429
 
 def CStringModeling : Checker<"CStringModeling">,
+  HelpText<"Responsible for essential modeling of cstring lengths. "
----------------
Szelethus wrote:
> What other aspects of c strings needs to be modeled? Is it only length? If so, how about we rename the checker to `CStringLengthModeling`?
For now I think the cstring length is enough.
I'm not sure if we will want to have other properties as well.
You are probably right.


================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:495
   ]>,
-  Dependencies<[CStringModeling]>,
+  Dependencies<[CStringChecker]>,
   Documentation<NotDocumented>,
----------------
Szelethus wrote:
> I dug around a bit, and found this commit as to why this was needed: rGe56167e8f87acf87a9de3d383752e18a738cf056. So this dependency is appropriate.
Interesting.
I was just doing a search & replace though :)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:74
 enum class ConcatFnKind { none = 0, strcat = 1, strlcat = 2 };
-class CStringChecker : public Checker< eval::Call,
-                                         check::PreStmt<DeclStmt>,
-                                         check::LiveSymbols,
-                                         check::DeadSymbols,
-                                         check::RegionChanges
-                                         > {
+class CStringChecker : public Checker<eval::Call> {
   mutable std::unique_ptr<BugType> BT_Null, BT_Bounds, BT_Overlap,
----------------
Szelethus wrote:
> This is somewhat orthogonal to the patch, but shouldn't precondition violations be reported at `preCall`?
That is the current behavior.
We should consider in the future using `preCall` if we refactor so relentlessly.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringLength.cpp:175-181
+void ento::registerCStringModeling(CheckerManager &Mgr) {
+  Mgr.registerChecker<CStringModeling>();
+}
+
+bool ento::shouldRegisterCStringModeling(const CheckerManager &) {
+  return true;
+}
----------------
Szelethus wrote:
> We traditionally put these on the bottom of the file -- I don't think this would upset the structure too much :)
I wanted to place the class definition as close as possible to the registration function.
I can move this though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84316