[PATCH] D98034: [clang-tidy] Use-after-move: Ignore moves inside a try_emplace.

[Martin Böhme via Phabricator via cfe-commits]

mboehme updated this revision to Diff 328491.
mboehme added a comment.

Responses to review comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98034/new/

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,22 @@
   }
 };
 
+// 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 @@
     container.empty();
   }
   {
-    std::map<int> container;
+    std::map<int, int> container;
     std::move(container);
     container.clear();
     container.empty();
@@ -800,7 +839,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
+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: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -398,7 +398,13 @@
                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(

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits