aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added a reviewer: aaron.ballman.
================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:20
@@ +19,3 @@
+
+static void ReplaceMoveWithForward(const UnresolvedLookupExpr *Callee,
+ const TemplateTypeParmType *TypeParmType,
----------------
Might as well pass `Context` by const reference rather than pointer.
================
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);
+ }
+ }
----------------
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).
================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:92
@@ +91,3 @@
+ Finder->addMatcher(
+ callExpr(callee(unresolvedLookupExpr().bind("lookup")),
+ argumentCountIs(1),
----------------
It might be a bit more clear if you made `isStdMove()` into a local AST matcher
and called it from here.
================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:119
@@ +118,3 @@
+
+ auto Diag = diag(FileMoveRange.getBegin(),
+ "forwarding reference passed to std::move(); did you mean "
----------------
Is there a reason why you can't just use `CallMove->getExprLoc()` instead of
toying with character ranges?
https://reviews.llvm.org/D22220
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
