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