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

[Aaron Ballman via cfe-commits]

aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added a reviewer: aaron.ballman.

================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:20
@@ +19,3 @@
+
+static void ReplaceMoveWithForward(const UnresolvedLookupExpr *Callee,
+                                   const TemplateTypeParmType *TypeParmType,
----------------
Might as well pass `Context` by const reference rather than pointer.

================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:43-53
@@ +42,13 @@
+    // another namespace).
+    const StringRef OriginalText =
+        Lexer::getSourceText(CallRange, SM, LangOpts);
+    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);
+    }
+  }
----------------
I'm not certain I understand the benefit to this. We know it's a call to std::move() from the standard namespace already, so why do we care about the original text? I think it's reasonable to replace any call to move() from the standard namespace with `::std::forward()`, so we should be able to remove the if statements and not have to go read the original source text from the source manager (which could involve, for instance, a query for that text over a slow network).

================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:92
@@ +91,3 @@
+  Finder->addMatcher(
+      callExpr(callee(unresolvedLookupExpr().bind("lookup")),
+               argumentCountIs(1),
----------------
It might be a bit more clear if you made `isStdMove()` into a local AST matcher and called it from here.

================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:119
@@ +118,3 @@
+
+  auto Diag = diag(FileMoveRange.getBegin(),
+                   "forwarding reference passed to std::move(); did you mean "
----------------
Is there a reason why you can't just use `CallMove->getExprLoc()` instead of toying with character ranges?


https://reviews.llvm.org/D22220