[PATCH] D28771: [Analyzer] Various fixes for the IteratorPastEnd checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Thanks for the update, most welcome!



================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:234
+        if (Call.getNumArgs() >= 1) {
+          const auto &InstCall = static_cast<const CXXInstanceCall &>(Call);
+          handleRandomIncrOrDecr(C, Func->getOverloadedOperator(),
----------------
We usually write these as
```
const auto *InstCall = cast<CXXInstanceCall>(&Call);
```
Mostly because our custom `cast<>` has an assertion inside.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:514
+    CheckerContext &C, OverloadedOperatorKind Op, const SVal &RetVal,
+    const SVal &LVal, const SVal &RVal, QualType RValType,
+    bool PreCheck) const {
----------------
I'd suggest renaming the parameters to "LHS" and "RHS" or something like that, so that not to confuse those with "lvalue" and "rvalue".


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:530
+  auto value = RVal;
+  if (auto loc = value.getAs<Loc>()) {
+    value = State->getRawSVal(*loc);
----------------
Is there a test case for this hack?

I'd also consider inspecting the AST (probably before passing the values to `handleRandomIncrOrDecr()`) and making the decision based on that. Because even though this pattern ("if a value is a loc and we expect a nonloc, do an extra dereference") is present in many places in the analyzer, in most of these places it doesn't work correctly (what if we try to discriminate between `int*` and `int*&`?).


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:553
+
+  // When increasing by positive or decreasing by negative an iterator past its
+  // end, then it is a bug. We check for bugs before the operator call.
----------------
I think incrementing `end()` by `0` is not a bug (?)


https://reviews.llvm.org/D28771