[PATCH] D30650: [clang-tidy] misc-use-after-move: Fix failing assertion

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

mboehme created this revision.
Herald added a subscriber: JDevlieghere.

I've added a test case that (without the fix) triggers the assertion,
which happens when a move happens in an implicitly called conversion
operator.

This patch also fixes nondeterministic behavior in the source code
location reported for the move when the move is constained in an init list;
this was causing buildbot failures in the previous attempt to submit
this patch (see https://reviews.llvm.org/D30569 and https://reviews.llvm.org/rL297004).


https://reviews.llvm.org/D30650

Files:
  clang-tidy/misc/UseAfterMoveCheck.cpp
  test/clang-tidy/misc-use-after-move.cpp


Index: test/clang-tidy/misc-use-after-move.cpp
===================================================================
--- test/clang-tidy/misc-use-after-move.cpp
+++ test/clang-tidy/misc-use-after-move.cpp
@@ -282,7 +282,7 @@
   S s{std::move(a)};
   a.foo();
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'a' used after it was moved
-  // CHECK-MESSAGES: [[@LINE-3]]:6: note: move occurred here
+  // CHECK-MESSAGES: [[@LINE-3]]:7: note: move occurred here
 }
 
 void lambdas() {
@@ -397,6 +397,21 @@
 }
 template void movedTypeIsDependentType<A>();
 
+// We handle the case correctly where the move consists of an implicit call
+// to a conversion operator.
+void implicitConversionOperator() {
+  struct Convertible {
+    operator A() && { return A(); }
+  };
+  void takeA(A a);
+
+  Convertible convertible;
+  takeA(std::move(convertible));
+  convertible;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'convertible' used after it was moved
+  // CHECK-MESSAGES: [[@LINE-3]]:9: note: move occurred here
+}
+
 // Using decltype on an expression is not a use.
 void decltypeIsNotUse() {
   A a;
Index: clang-tidy/misc/UseAfterMoveCheck.cpp
===================================================================
--- clang-tidy/misc/UseAfterMoveCheck.cpp
+++ clang-tidy/misc/UseAfterMoveCheck.cpp
@@ -384,6 +384,13 @@
       // the direct ancestor of the std::move() that isn't one of the node
       // types ignored by ignoringParenImpCasts().
       stmt(forEach(expr(ignoringParenImpCasts(CallMoveMatcher))),
+           // Don't allow an InitListExpr to be the moving call. An InitListExpr
+           // has both a syntactic and a semantic form, and the parent-child
+           // relationships are different between the two. This could cause an
+           // InitListExpr to be analyzed as the moving call in addition to the
+           // Expr that we actually want, resulting in two diagnostics with
+           // different code locations for the same move.
+           unless(initListExpr()),
            unless(expr(ignoringParenImpCasts(equalsBoundNode("call-move")))))
           .bind("moving-call"),
       this);
@@ -398,7 +405,7 @@
   const auto *MovingCall = Result.Nodes.getNodeAs<Expr>("moving-call");
   const auto *Arg = Result.Nodes.getNodeAs<DeclRefExpr>("arg");
 
-  if (!MovingCall)
+  if (!MovingCall || !MovingCall->getExprLoc().isValid())
     MovingCall = CallMove;
 
   Stmt *FunctionBody = nullptr;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D30650.90692.patch
Type: text/x-patch
Size: 2431 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170306/673ca56c/attachment.bin>