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

[Martin Böhme via cfe-commits]

mboehme marked 2 inline comments as done.

================
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".
----------------
aaron.ballman wrote:
> This code can be combined with the Global check below with something like:
> ```
> Diag << ..., Twine(NNS->getPrefix()->getKind() == NestedNameSpecifier::Global ? "::" : "") + "std::" + ForwardName);
> ```
It could -- but note that there is a third case that also needs to be handled here, namely that NNS has a prefix, but it's something other than "::". In other words, the code would need to go something like this:

```
if (!NNS->getPrefix() || NNS->getPrefix()->getKind() == NestedNameSpecifier::Global) {
  Diag << ..., Twine(NNS->getPrefix() ? "::" : "") + "std::" + ForwardName);
}
```

This repeats part of the check, and I feel it's more opaque than explicitly spelling out the two cases I'm interested in, at the cost of a little duplication.

================
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?");
+
----------------
aaron.ballman wrote:
> 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"
> I wonder if it makes more sense to word the diagnostic as an imperative rather than a question?

Good point -- done.

I've reworded your suggestion a bit:

- I've phrased this simply as "may unexpectedly cause lvalues to be moved". That's shorter and, I'm hoping, more accessible than talking about "indeterminate state".

- I've left out the reference to "perfect forwarding". If someone triggers this warning, chances are they won't know what it is anyway. They still have everything they need to find out about perfect forwarding; if they do a search for std::forward(), they should find relevant material.

What do you think?

================
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
----------------
aaron.ballman wrote:
> Uncertain whether we're doing this or not, but should we keep this list alphabetized?
I think it's a good idea -- done!


https://reviews.llvm.org/D22220