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

[Martin Böhme via cfe-commits]

mboehme marked 13 inline comments as done.

================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:20
@@ +19,3 @@
+
+static void ReplaceMoveWithForward(const UnresolvedLookupExpr *Callee,
+                                   const TemplateTypeParmType *TypeParmType,
----------------
sbenza wrote:
> aaron.ballman wrote:
> > Might as well pass `Context` by const reference rather than pointer.
> Function names start in lower case.
Sorry -- still getting used to LLVM naming conventions.

================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:30
@@ +29,3 @@
+          Callee->getLocStart(),
+          Lexer::getLocForEndOfToken(Callee->getLocEnd(), 0, SM, LangOpts)),
+      SM, LangOpts);
----------------
sbenza wrote:
> Isn't this just `CharSourceRange::getTokenRange(Callee->getLocStart(),Callee->getLocEnd())` ?
Thanks -- done.

================
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);
+    }
+  }
----------------
etienneb wrote:
> 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.
> why do we care about the original text?

See alexfh's comment -- my intent is to perserve the existing style in terms of whether "std" is globally qualified or not.

================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:83
@@ +82,3 @@
+          // same function...
+          references(templateTypeParmType(hasDeclaration(hasDeclContext(
+                                              functionDecl().bind("context"))))
----------------
sbenza wrote:
> Will this trigger on code like this:
> 
> 
> ```
> template <typename T>
> void F(T);
> 
> void G() { F<string&&>(std::move(str)); }
> ```
No, it won't -- I've added a test for this.

The reason it doesn't trigger is because the check only looks at the template itself, but not any instantiations (this is because the main matcher below matches for UnresolvedLookupExpr, which only exists in the template).

================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:92
@@ +91,3 @@
+  Finder->addMatcher(
+      callExpr(callee(unresolvedLookupExpr().bind("lookup")),
+               argumentCountIs(1),
----------------
etienneb wrote:
> 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.
That was my original plan, but there doesn't seem to be a way to do this with an AST matcher, and I'm not sure whether it's desired that this should change or what would be the best way to change it.

The main issue is that the decls() contained in a UnresolvedLookupExpr aren't children of the UnresolvedLookupExpr, and RecursiveASTVisitor doesn't contain any special-case logic to visit them either. (A consequence of this is that they don't show up when dumping the AST.) This means that it isn't possible to match for them using unresolvedLookupExpr(has(...)), for example.

This is the reason I went with an explicit test inside the check(). But if you have a suggestion on how this could be changed into an AST matcher in a way that's consistent with the overall design of AST matchers, I'd be open to that!

================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:119
@@ +118,3 @@
+
+  auto Diag = diag(FileMoveRange.getBegin(),
+                   "forwarding reference passed to std::move(); did you mean "
----------------
alexfh wrote:
> 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.
Multiple changes here:

  - Following alexfh's suggestion, I only disable the fix-it and not the whole diagnostic for the macro constructs in question (see corresponding change in the test).

  - It turns out that once I do this, the isValid() check in replaceMoveWithForward() already does the job of avoiding making invalid replacements on macros, so I can do without it here and indeed simply use CallMove->getExprLoc(). (I've also added another macro test case for additional coverage.)



================
Comment at: test/clang-tidy/misc-move-forwarding-reference.cpp:18
@@ +17,3 @@
+// Standard case.
+template <typename T, typename U> void f1(U &&SomeU) {
+  T SomeT(std::move(SomeU));
----------------
Added numbering for consistency.


https://reviews.llvm.org/D22220