[clang-tidy] Use-after-move: Ignore moves inside a try_emplace.
authorMartin Boehme <mboehme@google.com>
Fri, 5 Mar 2021 12:16:00 +0000 (13:16 +0100)
committerMartin Boehme <mboehme@google.com>
Fri, 5 Mar 2021 14:05:09 +0000 (15:05 +0100)
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.

Reviewed By: sammccall

Differential Revision: https://reviews.llvm.org/D98034

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 028fefa..136d8f8 100644 (file)
@@ -398,7 +398,13 @@ void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
                hasArgument(0, declRefExpr().bind("arg")),
                anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")),
                      hasAncestor(functionDecl().bind("containing-func"))),
-               unless(inDecltypeOrTemplateArg()))
+               unless(inDecltypeOrTemplateArg()),
+               // try_emplace is a common maybe-moving function that returns a
+               // bool to tell callers whether it moved. Ignore std::move inside
+               // try_emplace to avoid false positives as we don't track uses of
+               // the bool.
+               unless(hasParent(cxxMemberCallExpr(
+                   callee(cxxMethodDecl(hasName("try_emplace")))))))
           .bind("call-move");
 
   Finder->addMatcher(
index aab7cfd..8509292 100644 (file)
@@ -169,6 +169,10 @@ that a move always takes place:
 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
+is conservatively assumed not to move. 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 527c798..73ca59c 100644 (file)
@@ -32,6 +32,31 @@ struct weak_ptr {
   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(deque);
 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,22 @@ class IgnoreMemberVariables {
   }
 };
 
+// Ignore moves that happen in a try_emplace.
+void ignoreMoveInTryEmplace() {
+  {
+    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();
+  }
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // Tests involving control flow.
 
@@ -776,7 +815,7 @@ void standardContainerClearIsReinit() {
     container.empty();
   }
   {
-    std::map<int> container;
+    std::map<int, int> container;
     std::move(container);
     container.clear();
     container.empty();
@@ -800,7 +839,7 @@ void standardContainerClearIsReinit() {
     container.empty();
   }
   {
-    std::unordered_map<int> container;
+    std::unordered_map<int, int> container;
     std::move(container);
     container.clear();
     container.empty();