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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
