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

[Alexander Kornienko via cfe-commits]

alexfh added a subscriber: alexfh.

================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:36
@@ +35,3 @@
+        (llvm::Twine("forward<") + TypeParmType->getDecl()->getName() +
+         llvm::Twine(">"))
+            .str();
----------------
nit: The second `llvm::Twine` is not necessary.

================
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);
+    }
+  }
----------------
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.

================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:60
@@ +59,3 @@
+  for (const auto &Decl : UnresolvedLookup->decls()) {
+    const auto *TheFunctionTemplateDecl =
+        dyn_cast<FunctionTemplateDecl>(Decl->getUnderlyingDecl());
----------------
The variable name is rather uncommon for LLVM/Clang code (in particular, the `The` prefix). Maybe `FuncTemplateDecl`? Equally clear and somewhat shorter.

================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:119
@@ +118,3 @@
+
+  auto Diag = diag(FileMoveRange.getBegin(),
+                   "forwarding reference passed to std::move(); did you mean "
----------------
aaron.ballman wrote:
> Is there a reason why you can't just use `CallMove->getExprLoc()` instead of toying with character ranges?
I think, `makeFileCharRange` is needed. It helps finding the correct macro expansion level to insert the replacement on. In case the range is not on the same macro expansion level, the condition on the line 116 will stop the check from introducing changes to the code that are more likely to break it.

The only change I would suggest is to only disable the fix-it and not the whole diagnostic in case unsupported macro usage is detected.


https://reviews.llvm.org/D22220