[PATCH] D53812: [Analyzer] Iterator Checker - Forbid increments past the begin() and decrements past the end() of containers

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:420-465
     if (ChecksEnabled[CK_InvalidatedIteratorChecker] &&
         isAccessOperator(Func->getOverloadedOperator())) {
       // Check for any kind of access of invalidated iterator positions
       if (const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call)) {
         verifyAccess(C, InstCall->getCXXThisVal());
       } else {
         verifyAccess(C, Call.getArgSVal(0));
----------------
This is a bit of an organisational comment (and the compiler of course hopefully optimises this out), but could you, for the sake of code readability, break the if-elseif-elseif-elseif chain's common check out into an outer if, and only check for the inner parts? Thinking of something like this:

```
if (ChecksEnabled[CK_InvalidatedIteratorChecker])
{
   /* yadda */
}

if (ChecksEnabled[CK_IteratorRangeChecker])
{
  X* OOperator = Func->getOverloadedOperator();
  if (isIncrementOperator(OOperator))
  {
    /* yadda */
  } else if (isDecrementOperator(OOperator)) {
    /* yadda */
  }
}

/* etc. */ 
```


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1074
 
-  auto &SymMgr = C.getSymbolManager();
-  auto &SVB = C.getSValBuilder();
-  auto BinOp = (Op == OO_Plus || Op == OO_PlusEqual) ? BO_Add : BO_Sub;
-  const auto OldOffset = Pos->getOffset();
-  const auto intValue = value.getAs<nonloc::ConcreteInt>();
-  if (!intValue)
+  // Incremention or decremention by 0 is never bug
+  if (isZero(State, Value.castAs<NonLoc>()))
----------------
is never a, also `.` at the end


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1568
 
+IteratorPosition IteratorChecker::shiftPosition(CheckerContext &C,
+                                                OverloadedOperatorKind Op,
----------------
(Minor: I don't like the name of this function, `advancePosition` (akin to `std::advance`) sounds cleaner to my ears.)


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1575
+  auto &SVB = C.getSValBuilder();
+  auto BinOp = (Op == OO_Plus || Op == OO_PlusEqual) ? BO_Add : BO_Sub;
+  if (const auto IntDist = Distance.getAs<nonloc::ConcreteInt>()) {
----------------
For the sake of clarity, I think an assert should be introduced her, lest someone ends up shifting the position with `<=`.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:2329-2330
 
   // Out of range means less than the begin symbol or greater or equal to the
   // end symbol.
 
----------------
How does the introduction of `IncludeEnd` change this behaviour? What does the parameter refer to?


Repository:
  rC Clang

https://reviews.llvm.org/D53812