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

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:46-56
@@ +45,13 @@
+    // std::move(). This will hopefully prevent erroneous replacements if the
+    // code does unusual things (e.g. create an alias for std::move() in
+    // another namespace).
+    NestedNameSpecifier *NNS = Callee->getQualifier();
+    if (!NNS) {
+      // Called as "move" (i.e. presumably the code had a "using std::move;").
+      // We still conservatively put a "std::" in front of the forward because
+      // we don't know whether the code also had a "using std::forward;".
+      Diag << FixItHint::CreateReplacement(CallRange, "std::" + ForwardName);
+    } else if (const NamespaceDecl *Namespace = NNS->getAsNamespace()) {
+      if (Namespace->getName() == "std") {
+        if (!NNS->getPrefix()) {
+          // Called as "std::move".
----------------
This code can be combined with the Global check below with something like:
```
Diag << ..., Twine(NNS->getPrefix()->getKind() == NestedNameSpecifier::Global ? "::" : "") + "std::" + ForwardName);
```

================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:124-125
@@ +123,4 @@
+  auto Diag = diag(CallMove->getExprLoc(),
+                   "forwarding reference passed to std::move(); did you mean "
+                   "to use std::forward() instead?");
+
----------------
Given that the user is possibly in a confused state when they wrote `std::move()` rather than `std::forward()`, I wonder if it makes more sense to word the diagnostic as an imperative rather than a question? The diagnostic as it is doesn't really explain why `std::forward()` would be right while `std::move()` could be wrong.

Perhaps: "forwarding reference passed to `std::move()` may unexpectedly leave lvalue references in an indeterminate state; use `std::forward()` instead for perfect forwarding"

================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.h:27
@@ +26,3 @@
+///
+/// This has a consequence that is usually unwanted and possibly surprising: If
+/// the function that takes the forwarding reference as its parameter is called
----------------
"If" should not be capitalized here.

================
Comment at: docs/ReleaseNotes.rst:78
@@ -77,1 +77,3 @@
 
+- New `misc-move-forwarding-reference
+  <http://clang.llvm.org/extra/clang-tidy/checks/misc-move-forwarding-reference.html>`_ check
----------------
Uncertain whether we're doing this or not, but should we keep this list alphabetized?

================
Comment at: docs/clang-tidy/checks/misc-move-forwarding-reference.rst:32
@@ +31,3 @@
+
+Code like the example above is often written in the expectation that ``T&&``
+will always end up being an rvalue reference, no matter what type is deduced for
----------------
"is often written in the" -> "is sometimes written with the"


https://reviews.llvm.org/D22220