[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<int> container;
+    std::map<int, int> 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