From 934743f8f8dbd291b7288aa7cb5ad9ae0960ef3b Mon Sep 17 00:00:00 2001
From: Martin Bohme <mboehme@google.com>
Date: Fri, 14 Oct 2016 13:23:39 +0000
Subject: [PATCH] [clang-tidy] Add additional diagnostic to misc-use-after-move

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

Reviewers: hokein

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D25612

llvm-svn: 284235
---
 .../clang-tidy/misc/UseAfterMoveCheck.cpp     | 24 +++++++++++--------
 .../test/clang-tidy/misc-use-after-move.cpp   |  6 +++--
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/misc/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/misc/UseAfterMoveCheck.cpp
index f1485c8e77f5..bc21754abddb 100644
--- a/clang-tools-extra/clang-tidy/misc/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/UseAfterMoveCheck.cpp
@@ -562,18 +562,24 @@ void UseAfterMoveFinder::getReinits(
 }
 
 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);
   }
 }
 
@@ -625,8 +631,6 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) {
   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())
@@ -634,8 +638,8 @@ void UseAfterMoveCheck::check(const MatchFinder::MatchResult &Result) {
 
   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
diff --git a/clang-tools-extra/test/clang-tidy/misc-use-after-move.cpp b/clang-tools-extra/test/clang-tidy/misc-use-after-move.cpp
index 46fa651efb1f..f68c2fbb5db2 100644
--- a/clang-tools-extra/test/clang-tidy/misc-use-after-move.cpp
+++ b/clang-tools-extra/test/clang-tidy/misc-use-after-move.cpp
@@ -133,6 +133,7 @@ void moveAfterMove() {
       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
     }
   }
 }
@@ -391,7 +392,8 @@ void useAndMoveInLoop() {
     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);
     }
   }
@@ -586,7 +588,7 @@ void assignments(int i) {
   }
 }
 
-// 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 &);