[PATCH] D22220: [clang-tidy] Add check 'misc-move-forwarding-reference'

[Aaron Ballman via cfe-commits]

aaron.ballman added a subscriber: klimek.

================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:44-54
@@ +43,13 @@
+    if (OriginalText == "::std::move") {
+      Diag << FixItHint::CreateReplacement(CallRange, "::std::" + ForwardName);
+      // If the original text was simply "move", we conservatively still put a
+      // "std::" in front of the "forward". Rationale: Even if the code
+      // apparently had a "using std::move;", that doesn't tell us whether it
+      // also had a "using std::forward;".
+    } else if (OriginalText == "std::move" || OriginalText == "move") {
+      Diag << FixItHint::CreateReplacement(CallRange, "std::" + ForwardName);
+    }
+  }
+}
+
+// Returns whether the UnresolvedLookupExpr (potentially) refers to std::move().
----------------
I kind of thought that was going to be the case, but wanted to double-check. I agree with the notion, but disagree with the approach -- as @etienneb mentions, this code is not resilient enough to actually do the right thing. I don't think we should be going back to the original source text, but should instead be using the information the AST already tracks. This will perform better and do the right thing in all cases.

================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:93
@@ +92,3 @@
+               hasArgument(0, ignoringParenImpCasts(declRefExpr(
+                                  to(ForwardingReferenceParmMatcher)))))
+          .bind("call-move"),
----------------
I wonder if there's a reason for this behavior, or if it's simple a matter of not being needed previously and so it was never implemented. @sbenza or @klimek may know. I think we should be fixing the RecursiveASTVisitor, unless there is a valid reason not to (which there may be), though that would be a separate patch (and can happen after we land this one).


https://reviews.llvm.org/D22220