Szelethus added a comment.
Its very interesting to see how the idea of interestingness propagation through
`NoteTag`s works in practice -- I feel like many other checkers will need to
adopt something like this, and its great that we have the first use case
presented here. The patch from afar looks good, I only left some nits to ease
the readability for the uninitiated.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/DebugIteratorModeling.cpp:91-111
SVal V = C.getSVal(CE->getArg(0));
const auto *Pos = getIteratorPosition(State, V);
+ SVal Field = Default;
+
if (Pos) {
- State = State->BindExpr(CE, C.getLocationContext(), get(Pos));
- } else {
- State = State->BindExpr(CE, C.getLocationContext(), Default);
+ Field = get(Pos);
}
----------------
Now that we have multiple `SVal`s around, can we rename `V`? Also, I would
appreciate some comments. As I understand it, `ExprInspectionChecker` now marks
the arguments as interesting, so if we write this:
```lang=cpp
clang_analyzer_express(clang_analyzer_iterator_position(i2));
```
`clang_analyzer_iterator_position(i2)` will be interesting, and this function
propagates this interestingness to `i2`, correct?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/DebugIteratorModeling.cpp:103
+ C.getNoteTag([V, Field](PathSensitiveBugReport &BR) -> std::string {
+ auto *PSBR = cast<PathSensitiveBugReport>(&BR);
+ if (PSBR->isInteresting(Field)) {
----------------
We won't need this anymore :)
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:432
- reportBug(*Str, C);
+ reportBug(*Str, C, Optional<SVal>(ArgVal));
}
----------------
Why the need for the `Optional` stuff here?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:108-109
SVal It2 = UndefinedVal()) const;
+ const NoteTag *getInterestingnessPropagationTag(CheckerContext &C, SVal It1,
+ SVal It2) const;
void printState(raw_ostream &Out, ProgramStateRef State, const char *NL,
----------------
The usage of this function is very non-obvious, plenty of comments would be
appreciated here as well.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75677/new/
https://reviews.llvm.org/D75677
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
