mboehme created this revision.
mboehme added a reviewer: sammccall.
mboehme added a project: clang-tools-extra.
Herald added a subscriber: xazax.hun.
mboehme requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
We have no way to reason about the bool returned by try_emplace, so we
simply ignore any std::move()s that happen in a try_emplace argument.
A lot of the time in this situation, the code will be checking the
bool and doing something else if it turns out the value wasn't moved
into the map, and this has been causing false positives so far.
I don't currently have any intentions of handling "maybe move" functions
more generally.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D98034
Files:
clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
clang-tools-extra/docs/clang-tidy/checks/bugprone-use-after-move.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp
@@ -32,6 +32,31 @@
bool expired() const;
};
+template <typename T1, typename T2>
+struct pair {};
+
+template <typename Key, typename T>
+struct map {
+ struct iterator {};
+
+ map();
+ void clear();
+ bool empty();
+ template <class... Args>
+ pair<iterator, bool> try_emplace(const Key &key, Args &&...args);
+};
+
+template <typename Key, typename T>
+struct unordered_map {
+ struct iterator {};
+
+ unordered_map();
+ void clear();
+ bool empty();
+ template <class... Args>
+ pair<iterator, bool> try_emplace(const Key &key, Args &&...args);
+};
+
#define DECLARE_STANDARD_CONTAINER(name) \
template <typename T> \
struct name { \
@@ -55,11 +80,9 @@
DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(forward_list);
DECLARE_STANDARD_CONTAINER_WITH_ASSIGN(list);
DECLARE_STANDARD_CONTAINER(set);
-DECLARE_STANDARD_CONTAINER(map);
DECLARE_STANDARD_CONTAINER(multiset);
DECLARE_STANDARD_CONTAINER(multimap);
DECLARE_STANDARD_CONTAINER(unordered_set);
-DECLARE_STANDARD_CONTAINER(unordered_map);
DECLARE_STANDARD_CONTAINER(unordered_multiset);
DECLARE_STANDARD_CONTAINER(unordered_multimap);
@@ -483,6 +506,35 @@
}
};
+// Ignore moves that happen in a try_emplace.
+void try_emplace() {
+ {
+ std::map<int, A> amap;
+ A a;
+ amap.try_emplace(1, std::move(a));
+ a.foo();
+ }
+ {
+ std::unordered_map<int, A> amap;
+ A a;
+ amap.try_emplace(1, std::move(a));
+ a.foo();
+ }
+ {
+ // Don't make any assumptions about methods called try_emplace that don't
+ // belong to a standard container.
+ struct map {
+ void try_emplace(int, A &&);
+ };
+ map mymap;
+ A a;
+ mymap.try_emplace(1, std::move(a));
+ a.foo();
+ // CHECK-NOTES: [[@LINE-1]]:5: warning: 'a' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:11: note: move occurred here
+ }
+}
+
////////////////////////////////////////////////////////////////////////////////
// Tests involving control flow.
@@ -776,7 +828,7 @@
container.empty();
}
{
- std::map<int> container;
+ std::map<int, int> container;
std::move(container);
container.clear();
container.empty();
@@ -800,7 +852,7 @@
container.empty();
}
{
- std::unordered_map<int> container;
+ std::unordered_map<int, int> container;
std::move(container);
container.clear();
container.empty();
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-use-after-move.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/bugprone-use-after-move.rst
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-use-after-move.rst
@@ -169,6 +169,10 @@
The check will assume that the last line causes a move, even though, in this
particular case, it does not. Again, this is intentional.
+There is one special case: A call to ``std::move`` inside a ``try_emplace`` call
+on a standard map type is ignored. This is to avoid spurious warnings, as the
+check has no way to reason about the ``bool`` returned by ``try_emplace``.
+
When analyzing the order in which moves, uses and reinitializations happen (see
section `Unsequenced moves, uses, and reinitializations`_), the move is assumed
to occur in whichever function the result of the ``std::move`` is passed to.
Index: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -393,12 +393,22 @@
}
void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
+ auto StandardMapMatcher =
+ hasType(hasUnqualifiedDesugaredType(recordType(hasDeclaration(
+ cxxRecordDecl(hasAnyName("::std::map", "::std::unordered_map"))))));
+
auto CallMoveMatcher =
callExpr(callee(functionDecl(hasName("::std::move"))), argumentCountIs(1),
hasArgument(0, declRefExpr().bind("arg")),
anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")),
hasAncestor(functionDecl().bind("containing-func"))),
- unless(inDecltypeOrTemplateArg()))
+ unless(inDecltypeOrTemplateArg()),
+ // Ignore std::move()s inside a try_emplace() call to avoid false
+ // positives as we can't reason about the bool returned by
+ // try_emplace.
+ unless(hasParent(cxxMemberCallExpr(
+ on(expr(declRefExpr(), StandardMapMatcher)),
+ callee(cxxMethodDecl(hasName("try_emplace")))))))
.bind("call-move");
Finder->addMatcher(
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
