[PATCH] D29151: [clang-tidy] Add misc-invalidated-iterators check.
Piotr Padlewski via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Jan 25 15:44:03 PST 2017
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
More information about the cfe-commits
mailing list