[PATCH] D25660: [Analyzer] Checker for iterators dereferenced beyond their range.

[Aleksei Sidorin via cfe-commits]

a.sidorin added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:209
+                                           CheckerContext &C) const {
+  const auto *Func = Call.getDecl()->getAsFunction();
+  if (Func->isOverloadedOperator()) {
----------------
As I remember, `PostCall` is also called for ObjC calls like `ObjCMethodCall` which may not have `FunctionDecl` as their callee. So, `Func` may be a nullptr and needs a check.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:156
+    if (isAccessOperator(Func->getOverloadedOperator())) {
+      if (Func->isCXXInstanceMember()) {
+        const auto &InstCall = static_cast<const CXXInstanceCall &>(Call);
----------------
This can be written some shorter: `if (const auto *InstCall = dyn_cast<CXXInstance>(&Call)`


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:219
+
+  assert(LCtx->getKind() == LocationContext::StackFrame &&
+         "Function does not begin with stack frame context");
----------------
`isa<StackFrameContext>(LCtx)`?
And `cast<>` below already does the same check with an assertion.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:258
+  auto State = C.getState();
+  const auto *LCtx = C.getPredecessor()->getLocationContext();
+
----------------
Just `C.getLocationContext()`?


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:305
+  auto RegionMap = State->get<IteratorRegionMap>();
+  for (auto I = RegionMap.begin(), E = RegionMap.end(); I != E; ++I) {
+    if (!SR.isLiveRegion(I->first)) {
----------------
This loop may be C++11-fied.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:322
+                                                   bool Assumption) const {
+  const auto *SE = Cond.getAsSymExpr();
+  if (!SE)
----------------
What will happen if we compare two iterators related to different containers? I guess the result will be undefined but I'm not sure if  we can track it in this checker without referencing the owning container. Let's leave this code as-is but I think this choice deserves a comment.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:337
+  const auto *RPos = getIteratorPosition(State, Right);
+  if (LPos && !RPos) {
+    if ((LPos->isInRange() && ((Opc == BO_EQ) == Assumption)) ||
----------------
Maybe we should just swap Rhs and Lhs if LPos is null? So, we can avoid code duplication.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:423
+
+void IteratorPastEndChecker::handleComparison(CheckerContext &C,
+                                              const SVal &LVal,
----------------
What will happen if we write something like this:
```
bool Eq1 = it1 == it2;
bool Eq2 = it3 == it4;
if (Eq1) {...}?
```

As I understand, we'll assume the second condition instead of first.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:454
+  if (Pos && Pos->isOutofRange()) {
+    State = setIteratorPosition(State, Val, IteratorPosition::getInRange());
+    C.addTransition(State);
----------------
I'm not sure it's totally correct. `--` for `begin()` will give us out-of-range iterator. According to header description, we're catching just "past-end" iterators, but this is confusing a bit for me.
Moreover, if we're out of end() in multiple positions, a single `--` will not make the iterator valid again. You use a good conservative approach, but could you please add a comment describing it?


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:571
+  auto &svalBuilder = C.getSValBuilder();
+  const auto *LCtx = C.getPredecessor()->getLocationContext();
+
----------------
Just `C.getLocationContext()`.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:573
+
+  auto RetVal = svalBuilder.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount());
+  auto SecondParam = state->getSVal(CE->getArg(1), C.getLocationContext());
----------------
You can use overload which does not require the tag.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:574
+  auto RetVal = svalBuilder.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount());
+  auto SecondParam = state->getSVal(CE->getArg(1), C.getLocationContext());
+
----------------
getLocationContext => LCtx 


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:605
+static bool isIteratorType(const QualType &Type) {
+  const auto *CRD = Type->getUnqualifiedDesugaredType()->getAsCXXRecordDecl();
+  return isIterator(CRD);
----------------
A common way of defining iterator types is just their declaration as pointers. I'm not sure this code will work well in such cases.
You can see some example in LLVM containers like SmallVector, where iterators are declared in the following way:

```
  typedef T *iterator;
  typedef const T *const_iterator;

```


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:618
+
+  bool CopyC = false, CopyA = true, Dest = false, PreIncr = false,
+       PostIncr = false, Deref = false;
----------------
HasCopyCtor, HasCopyAssign, etc.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:620
+       PostIncr = false, Deref = false;
+  for (const auto *M : CRD->methods()) {
+    if (const auto *C = dyn_cast<CXXConstructorDecl>(M)) {
----------------
We usually prefer informative names like "Method" or "Ctor".


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp:623
+      if (C->isCopyConstructor()) {
+        CopyC = !C->isDeleted() && C->getAccess() == AS_public;
+      }
----------------
There was a comment. Phabricator disallows me to delete my own comments so I was forced to edit it. Nevermind.


https://reviews.llvm.org/D25660