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

[Martin Böhme via cfe-commits]

mboehme marked 7 inline comments as done.

================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:93
@@ +92,3 @@
+               hasArgument(0, ignoringParenImpCasts(declRefExpr(
+                                  to(ForwardingReferenceParmMatcher)))))
+          .bind("call-move"),
----------------
sbenza wrote:
> aaron.ballman wrote:
> > I wonder if there's a reason for this behavior, or if it's simple a matter of not being needed previously and so it was never implemented. @sbenza or @klimek may know. I think we should be fixing the RecursiveASTVisitor, unless there is a valid reason not to (which there may be), though that would be a separate patch (and can happen after we land this one).
> Even if the nodes are not visited through the recursive visitor, you can always have a matcher for it.
> Eg: `hasAnyConstructorInitializer` / `cxxCtorInitializer`.
> 
> But what node are you trying to visit here?
> The only node I see is `NamingClass`, which is not really a child node.
> Like the referred `Decl` in a `DeclRefExpr` is not a child either. You can't use `has()` there, you have to use `to()`.
I've now reworked this to use new matchers (in review in D23004). Thanks for the comments, which finally got me on the right track!

> But what node are you trying to visit here?

I'm trying to visit the decls() in an OverloadExpr (this happened in isStdMove(), which is now deleted).

As you note, these intentionally aren't children of the OverloadExpr (so it doesn't make sense for the RecursiveASTMatcher to visit them), but it still makes sense to have a matchers for them -- which I've now added.

================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:45-55
@@ +44,13 @@
+      // 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".
+          Diag << FixItHint::CreateReplacement(CallRange,
+                                               "std::" + ForwardName);
+        } else if (NNS->getPrefix()->getKind() == NestedNameSpecifier::Global) {
+          // Called as "::std::move".
+          Diag << FixItHint::CreateReplacement(CallRange,
+                                               "::std::" + ForwardName);
----------------
I've changed the code to look at the NestedNameSpecifiers instead of looking at the original source text.

> But, at the same time, the check should be bomb proof for ugly cases like [snip]

Apologies, I misunderstood -- I thought you were saying it should be bomb proof in the sense that it shouldn't suggest any replacement (and that's why I marked your comment done). Having read aaron.ballman's comment, I now realize what you meant is that the check should be bomb proof in the sense that it should create a correct fix even for such "ugly" cases.


https://reviews.llvm.org/D22220