[clang-tidy] Add additional diagnostic to misc-use-after-move

[mboehme]

Summary

This adds a diagnostic to the misc-use-after-move check that is output when the
use happens on a later loop iteration than the move, for example:

A a;
for (int i = 0; i < 10; ++i) {
  a.foo();
  std::move(a);
}

This situation can be confusing to users because, in terms of source code
location, the use is above the move. This can make it look as if the warning
is a false positive, particularly if the loop is long but the use and move are
close together.

In cases like these, misc-use-after-move will now output an additional
diagnostic:

a.cpp:393:7: note: the use happens in a later loop iteration than the move

[hokein]

The code looks good.

clang-tidy/misc/UseAfterMoveCheck.cpp

579

A space after if.

[mboehme]

• Responses to reviewer comments

clang-tidy/misc/UseAfterMoveCheck.cpp

for (const auto &Match : Matches) {
         if (TheDeclRef)
           DeclRefs->insert(TheDeclRef);
       }
     }
   }
 }
 
 static void emitDiagnostic(const Expr *MovingCall,
-                           const ValueDecl *MovedVariable,
+                           const DeclRefExpr *MoveArg,
                            const UseAfterMove &Use, ClangTidyCheck *Check,
                            ASTContext *Context) {
-  Check->diag(Use.DeclRef->getExprLoc(), "'%0' used after it was moved")
-      << MovedVariable->getName();
-  Check->diag(MovingCall->getExprLoc(), "move occurred here",
-              DiagnosticIDs::Note);
+  SourceLocation UseLoc = Use.DeclRef->getExprLoc();
+  SourceLocation MoveLoc = MovingCall->getExprLoc();
+
+  Check->diag(UseLoc, "'%0' used after it was moved")
+      << MoveArg->getDecl()->getName();
+  Check->diag(MoveLoc, "move occurred here", DiagnosticIDs::Note);
   if (Use.EvaluationOrderUndefined) {
-    Check->diag(Use.DeclRef->getExprLoc(),
+    Check->diag(UseLoc,
                 "the use and move are unsequenced, i.e. there is no guarantee "
                 "about the order in which they are evaluated",
                 DiagnosticIDs::Note);
+  } else if (UseLoc < MoveLoc || Use.DeclRef == MoveArg) {
+    Check->diag(UseLoc,
+                "the use happens in a later loop iteration than the move",
+                DiagnosticIDs::Note);
   }
 }
 
 void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus11)
     return;
 
   auto StandardSmartPointerTypeMatcher = hasType(
void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) {
   Stmt *FunctionBody = nullptr;
   if (ContainingLambda)
     FunctionBody = ContainingLambda->getBody();
   else if (ContainingFunc)
     FunctionBody = ContainingFunc->getBody();
   else
     return;
 
-  const ValueDecl *MovedVariable = Arg->getDecl();
-
   // Ignore the std::move if the variable that was passed to it isn't a local
   // variable.
   if (!Arg->getDecl()->getDeclContext()->isFunctionOrMethod())
     return;
 
   UseAfterMoveFinder finder(Result.Context);
   UseAfterMove Use;
-  if (finder.find(FunctionBody, MovingCall, MovedVariable, &Use))
-    emitDiagnostic(MovingCall, MovedVariable, Use, this, Result.Context);
+  if (finder.find(FunctionBody, MovingCall, Arg->getDecl(), &Use))
+    emitDiagnostic(MovingCall, Arg, Use, this, Result.Context);
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

test/clang-tidy/misc-use-after-move.cpp

void moveAfterMove() {
   // This is also true if the move itself turns into the use on the second loop
   // iteration.
   {
     A a;
     for (int i = 0; i < 10; ++i) {
       std::move(a);
       // CHECK-MESSAGES: [[@LINE-1]]:17: warning: 'a' used after it was moved
       // CHECK-MESSAGES: [[@LINE-2]]:7: note: move occurred here
+      // CHECK-MESSAGES: [[@LINE-3]]:17: note: the use happens in a later loop
     }
   }
 }
 
 // Checks also works on function parameters that have a use-after move.
 void parameters(A a) {
   std::move(a);
   a.foo();

 void useAndMoveInLoop() {
   // Warn about use-after-moves if they happen in a later loop iteration than
   // the std::move().
   {
     A a;
     for (int i = 0; i < 10; ++i) {
       a.foo();
       // CHECK-MESSAGES: [[@LINE-1]]:7: warning: 'a' used after it was moved
-      // CHECK-MESSAGES: [[@LINE+1]]:7: note: move occurred here
+      // CHECK-MESSAGES: [[@LINE+2]]:7: note: move occurred here
+      // CHECK-MESSAGES: [[@LINE-3]]:7: note: the use happens in a later loop
       std::move(a);
     }
   }
   // However, this case shouldn't be flagged -- the scope of the declaration of
   // 'a' is important.
   {
     for (int i = 0; i < 10; ++i) {
       A a;
// to.
     if (i > 5) {
       a.foo();
       // CHECK-MESSAGES: [[@LINE-1]]:7: warning: 'a' used after it was moved
       // CHECK-MESSAGES: [[@LINE-7]]:5: note: move occurred here
     }
   }
 }
 
-// Passing the object to a function through a non-const pointer or refernce
+// Passing the object to a function through a non-const pointer or reference
 // counts as a re-initialization.
 void passByNonConstPointer(A *);
 void passByNonConstReference(A &);
 void passByNonConstPointerIsReinit() {
   {
     A a;
     std::move(a);
     passByNonConstPointer(&a);