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

[Etienne Bergeron via cfe-commits]

etienneb added a subscriber: etienneb.
etienneb added a comment.

thx for the check


================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:25
@@ +24,3 @@
+  const SourceManager &SM = Context->getSourceManager();
+  const LangOptions LangOpts = Context->getLangOpts();
+
----------------
nit: reference?

================
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);
+    }
+  }
----------------
alexfh wrote:
> aaron.ballman wrote:
> > 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).
> I think, the value of this is to maintain consistency with the existing code in terms of whether the `std` namespace should be globally qualified or not. Changing `std::move` to `::std::forward` would sometimes be unwelcome, if the code doesn't use `::std` otherwise.
I agree with alex, it's better to keep code consistency (programmer intend).
But, at the same time, the check should be bomb proof for ugly cases like:
```
  "std:: move"
   ":: std    ::    move",
```

For the moment, the code seems to propose a fix only when it's a known case,
and a warning is emitted in every cases.

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

The match will also be more precise.


https://reviews.llvm.org/D22220