[PATCH] D29151: [clang-tidy] Add misc-invalidated-iterators check.

[Piotr Padlewski via Phabricator via cfe-commits]

Prazek added inline comments.


================
Comment at: clang-tidy/misc/InvalidatedIteratorsCheck.cpp:61
+      cxxMemberCallExpr(has(memberExpr(hasDeclaration(cxxMethodDecl(hasAnyName(
+                                           "push_back", "emplace_back", "clear",
+                                           "insert", "emplace"))),
----------------
Please tests for all of this functions


================
Comment at: clang-tidy/misc/InvalidatedIteratorsCheck.cpp:92
+  const auto InsertResult =
+      CanFuncInvalidateMemo.insert(std::make_pair(MemoTuple, false));
+  assert(InsertResult.second);
----------------
.insert({MemoTuple, false})


================
Comment at: clang-tidy/misc/InvalidatedIteratorsCheck.cpp:99
+    // body; it possibly invalidates our iterators.
+    return (MemoIter->second = true);
+  }
----------------
I guess this might be to optimistic assumption. Normally we should be pesimistic to not introduce false positives. I will run thi check on llvm code base with SmallVector instead of std::vector and will see.



================
Comment at: clang-tidy/misc/InvalidatedIteratorsCheck.cpp:175-179
+  const std::unique_ptr<CFG> TheCFG =
+      CFG::buildCFG(nullptr, FuncBody, Result.Context, Options);
+  const std::unique_ptr<StmtToBlockMap> BlockMap(
+      new StmtToBlockMap(TheCFG.get(), Result.Context));
+  const std::unique_ptr<ExprSequence> Sequence(
----------------
const auto BlockMap = std::make_unique<StmtToBlockMap>(TheCFG.get(), Result.Context)

etc.


================
Comment at: clang-tidy/misc/InvalidatedIteratorsCheck.cpp:182-184
+  if (!Block) {
+    return;
+  }
----------------
remove extra braces


================
Comment at: docs/clang-tidy/checks/misc-invalidated-iterators.rst:18
+  vec.push_back(2017);
+  ref++;  // Incorrect - 'ref' might have been invalidated at 'push_back'.
+
----------------
suggest changing it to 
ref = 42;

When first reading I thought that it is iterator


================
Comment at: test/clang-tidy/misc-invalidated-iterators.cpp:11-17
+  class iterator {
+    Type *data;
+
+  public:
+    iterator(Type *ptr) : data(ptr) {}
+    Type &operator*() { return *data; }
+  };
----------------
iterator in vector is just raw pointer, so I guess you can replace it with

using iterator = Type*;


================
Comment at: test/clang-tidy/misc-invalidated-iterators.cpp:29
+}
+
+// Correct std::vector use.
----------------
Few testcases:

- passing vector as pointer
- passing vector as copy, it shouldn't care what happens
to modyfied vector.
- modyfing vector inside loop - does it finds it? like:
  for (auto &ref : vec) {
    vec.push_back(42);
    ref = 42;
  }



================
Comment at: test/clang-tidy/misc-invalidated-iterators.cpp:125
+  // CHECK-MESSAGES: :[[@LINE-4]]:5: note: possible place of invalidation
+}
----------------
add new line to file end


Repository:
  rL LLVM

https://reviews.llvm.org/D29151