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

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

mboehme added a comment.

First of all, sorry for submitting this change prematurely. I don't contribute 
to LLVM/Clang regularly and forgot that reviews should be kept open for a while 
after a reviewer has approved them to give others a chance to chime in. I 
jumped the gun here -- apologies.

In D98034#2606642 , @njames93 wrote:

> In D98034#2606535 , @mboehme wrote:
>
>> In D98034#2606488 , @njames93 wrote:
>>
>>> While `try_emplace` is a special case, it's may not be the only special 
>>> case in someone's codebase, this should be extended with options to let 
>>> users handle their special containers.
>>
>> Sorry, I didn't see your comment before submitting.
>>
>> I agree that there may be other cases of "maybe move" functions, but 
>> try_emplace seems to be the most common case. Handling "maybe move" 
>> functions more general would require some way of annotating them; I felt it 
>> was useful to get the try_emplace case handled first.
>
> The canonical way of doing this in clang-tidy is to add an option to the 
> check which takes a semi-colon delimited list.
> `clang::tidy::utils::options::parseStringList` can then turn that into a 
> vector of string.
> Annoyingly the hasAnyName matcher needs a vector of StringRef's so most 
> checks do something like this
>
>   hasAnyName(SmallVector(
> StringLikeClasses.begin(), StringLikeClasses.end())
>
> As you are reading options, you will need to override the storeOptions method 
> in the check to then store the value read from config, `serializeStringList` 
> is your friend here.
> The documentation will also need updating to describe the option and what it 
> does.

I think I shouldn't just use a `functionDecl(hasAnyName())` matcher with a 
string list; here's why.

In the case of `try_emplace`, it makes sense to match on any method (or even a 
free function) with that name. I was originally restricting myself to only the 
standard map types, but as @sammccall pointed out to me, someone using this 
pretty unique name in their own code is very likely to be using the same 
semantics as the standard map types.

This may not be true for all methods that a user might want to specify though. 
Say a user has

  class MyClass {
  public:
bool Insert(OtherClass &&);
  };

where, as for `try_emplace`, the boolean return value says whether the 
insertion actually happened.

We wouldn't simply want to ignore `std::move` arguments on any method called 
`Insert`. It's likely that there are other methods called `Insert` in the 
codebase that take an rvalue reference and move from it unconditionally, and we 
want the check to look at those.

So we'd want the user to be able to specify the qualified name 
`MyClass::Insert` as the function to ignore. This makes the matcher a bit more 
interesting as we'd need to work out when seeing a string like this whether 
`MyClass` is a class name or a namespace name. Or we might simply do something 
like

  anyOf(
  // maybe `MyClass` is a namespace...
  functionDecl(hasName("MyClass::Insert")),
  // ...or maybe it's a class
  cxxMethodDecl(ofClass(hasName("MyClass")), hasName("Insert"))
  )

I assume there isn't an existing matcher that does this -- or are you aware of 
one?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98034

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

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D98034#2606535 , @mboehme wrote:

> In D98034#2606488 , @njames93 wrote:
>
>> While `try_emplace` is a special case, it's may not be the only special case 
>> in someone's codebase, this should be extended with options to let users 
>> handle their special containers.
>
> Sorry, I didn't see your comment before submitting.
>
> I agree that there may be other cases of "maybe move" functions, but 
> try_emplace seems to be the most common case. Handling "maybe move" functions 
> more general would require some way of annotating them; I felt it was useful 
> to get the try_emplace case handled first.

The canonical way of doing this in clang-tidy is to add an option to the check 
which takes a semi-colon delimited list.
`clang::tidy::utils::options::parseStringList` can then turn that into a vector 
of string.
Annoyingly the hasAnyName matcher needs a vector of StringRef's so most checks 
do something like this

  hasAnyName(SmallVector(
StringLikeClasses.begin(), StringLikeClasses.end())

As you are reading options, you will need to override the storeOptions method 
in the check to then store the value read from config, `serializeStringList` is 
your friend here.
The documentation will also need updating to describe the option and what it 
does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98034

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

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

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

mboehme added a comment.

In D98034#2606488 , @njames93 wrote:

> While `try_emplace` is a special case, it's may not be the only special case 
> in someone's codebase, this should be extended with options to let users 
> handle their special containers.

Sorry, I didn't see your comment before submitting.

I agree that there may be other cases of "maybe move" functions, but 
try_emplace seems to be the most common case. Handling "maybe move" functions 
more general would require some way of annotating them; I felt it was useful to 
get the try_emplace case handled first.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98034

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

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

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

This revision was not accepted when it landed; it landed in state "Needs 
Revision".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
mboehme marked 3 inline comments as done.
Closed by commit rGe67d91faec21: [clang-tidy] Use-after-move: Ignore moves 
inside a try_emplace. (authored by mboehme).

Repository:
  rG LLVM Github Monorepo

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 
+struct pair {};
+
+template 
+struct map {
+  struct iterator {};
+
+  map();
+  void clear();
+  bool empty();
+  template 
+  pair try_emplace(const Key , Args &&...args);
+};
+
+template 
+struct unordered_map {
+  struct iterator {};
+
+  unordered_map();
+  void clear();
+  bool empty();
+  template 
+  pair try_emplace(const Key , Args &&...args);
+};
+
 #define DECLARE_STANDARD_CONTAINER(name) \
   template   \
   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 amap;
+A a;
+amap.try_emplace(1, std::move(a));
+a.foo();
+  }
+  {
+std::unordered_map amap;
+A a;
+amap.try_emplace(1, std::move(a));
+a.foo();
+  }
+}
+
 
 // Tests involving control flow.
 
@@ -776,7 +815,7 @@
 container.empty();
   }
   {
-std::map container;
+std::map container;
 std::move(container);
 container.clear();
 container.empty();
@@ -800,7 +839,7 @@
 container.empty();
   }
   {
-std::unordered_map container;
+std::unordered_map 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

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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D98034#2606488 , @njames93 wrote:

> While `try_emplace` is a special case, it's may not be the only special case 
> in someone's codebase, this should be extended with options to let users 
> handle their special containers.




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

https://reviews.llvm.org/D98034

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

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

[Nathan James via Phabricator via cfe-commits]

njames93 requested changes to this revision.
njames93 added a comment.
This revision now requires changes to proceed.

While `try_emplace` is a special case, it's may not be the only special case in 
someone's codebase, this should be behind extended with options to let users 
handle their special containers.




Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:406
+   // the bool.
+   unless(hasParent(cxxMemberCallExpr(
+   callee(cxxMethodDecl(hasName("try_emplace")))

Going with a custom list of bad names I'd suggest matching on function decls 
instead of method decls. 


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

https://reviews.llvm.org/D98034

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

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

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

mboehme marked 5 inline comments as done.
mboehme added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:396
 void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
+  auto StandardMapMatcher =
+  hasType(hasUnqualifiedDesugaredType(recordType(hasDeclaration(

sammccall wrote:
> I'd suggest dropping this condition.
> 
> Custom containers often emulate the standard library (e.g. llvm::DenseMap 
> implements try_emplace, as do the absl maps and I found several other 
> examples).
> 
> It seems very unlikely to encounter a try_emplace function without these 
> semantics.
> (The only case I can imagine is if for some reason they didn't bother to 
> implement the bool return value, so there was no way to know whether 
> emplacement happened. False negative there doesn't seem bad).
Thanks -- I wasn't sure myself whether I should be so restrictive, and you make 
some very convincing points. Done!



Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:406
+   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

sammccall wrote:
> Maybe explain why try_emplace is special?
> 
> try_emplace is a common maybe-moving function that returns a bool that tells 
> callers whether it moved.
> Ignore std::move inside try_emplace to avoid false positives, as we don't 
> track uses of the bool.
I've stolen your wording -- thanks!



Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:409
+   // try_emplace.
+   unless(hasParent(cxxMemberCallExpr(
+   on(expr(declRefExpr(), StandardMapMatcher)),

sammccall wrote:
> i'm trying to think if there's an important case where the call isn't the 
> immediate parent of the move and yet we're still maybe-moving... but I can't 
> think of one. (Just mentioning in case one occurs to you)
Yeah, I thought about that myself but couldn't come up with anything.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:831
   {
-std::map container;
+std::map container;
 std::move(container);

sammccall wrote:
> hahaha :-)
Yeah, I was being a bit sloppy with my definition of what a map is...


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

https://reviews.llvm.org/D98034

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

[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 
+struct pair {};
+
+template 
+struct map {
+  struct iterator {};
+
+  map();
+  void clear();
+  bool empty();
+  template 
+  pair try_emplace(const Key , Args &&...args);
+};
+
+template 
+struct unordered_map {
+  struct iterator {};
+
+  unordered_map();
+  void clear();
+  bool empty();
+  template 
+  pair try_emplace(const Key , Args &&...args);
+};
+
 #define DECLARE_STANDARD_CONTAINER(name) \
   template   \
   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 amap;
+A a;
+amap.try_emplace(1, std::move(a));
+a.foo();
+  }
+  {
+std::unordered_map amap;
+A a;
+amap.try_emplace(1, std::move(a));
+a.foo();
+  }
+}
+
 
 // Tests involving control flow.
 
@@ -776,7 +815,7 @@
 container.empty();
   }
   {
-std::map container;
+std::map container;
 std::move(container);
 container.clear();
 container.empty();
@@ -800,7 +839,7 @@
 container.empty();
   }
   {
-std::unordered_map container;
+std::unordered_map 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

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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Nice! Only substantive suggestion: I think requiring the base type to be 
exactly a standard map type is too conservative.




Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:396
 void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
+  auto StandardMapMatcher =
+  hasType(hasUnqualifiedDesugaredType(recordType(hasDeclaration(

I'd suggest dropping this condition.

Custom containers often emulate the standard library (e.g. llvm::DenseMap 
implements try_emplace, as do the absl maps and I found several other examples).

It seems very unlikely to encounter a try_emplace function without these 
semantics.
(The only case I can imagine is if for some reason they didn't bother to 
implement the bool return value, so there was no way to know whether 
emplacement happened. False negative there doesn't seem bad).



Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:406
+   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

Maybe explain why try_emplace is special?

try_emplace is a common maybe-moving function that returns a bool that tells 
callers whether it moved.
Ignore std::move inside try_emplace to avoid false positives, as we don't track 
uses of the bool.



Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:409
+   // try_emplace.
+   unless(hasParent(cxxMemberCallExpr(
+   on(expr(declRefExpr(), StandardMapMatcher)),

i'm trying to think if there's an important case where the call isn't the 
immediate parent of the move and yet we're still maybe-moving... but I can't 
think of one. (Just mentioning in case one occurs to you)



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-use-after-move.rst:173
+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``.

s/ignored/conservatively assumed *not* to move/?

(It wasn't obvious to me when reading how "ignored" in the implementation 
translates to the domain)



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:510
+// Ignore moves that happen in a try_emplace.
+void try_emplace() {
+  {

calling this function exactly try_emplace seems gratuitiously confusing - since 
the condition we're changing here is "calls inside a function named 
try_emplace", just with a different definition of "inside"...



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-use-after-move.cpp:831
   {
-std::map container;
+std::map container;
 std::move(container);

hahaha :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98034

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

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

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

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 
+struct pair {};
+
+template 
+struct map {
+  struct iterator {};
+
+  map();
+  void clear();
+  bool empty();
+  template 
+  pair try_emplace(const Key , Args &&...args);
+};
+
+template 
+struct unordered_map {
+  struct iterator {};
+
+  unordered_map();
+  void clear();
+  bool empty();
+  template 
+  pair try_emplace(const Key , Args &&...args);
+};
+
 #define DECLARE_STANDARD_CONTAINER(name) \
   template   \
   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 amap;
+A a;
+amap.try_emplace(1, std::move(a));
+a.foo();
+  }
+  {
+std::unordered_map 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 container;
+std::map container;
 std::move(container);
 container.clear();
 container.empty();
@@ -800,7 +852,7 @@
 container.empty();
   }
   {
-std::unordered_map container;
+std::unordered_map 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 =