etienneb added a subscriber: etienneb.
etienneb added a comment.
thx for the check
================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:25
@@ +24,3 @@
+ const SourceManager &SM = Context->getSourceManager();
+ const LangOptions LangOpts = Context->getLangOpts();
+
----------------
nit: reference?
================
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);
+ }
+ }
----------------
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.
================
Comment at: clang-tidy/misc/MoveForwardingReferenceCheck.cpp:92
@@ +91,3 @@
+ Finder->addMatcher(
+ callExpr(callee(unresolvedLookupExpr().bind("lookup")),
+ argumentCountIs(1),
----------------
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.
https://reviews.llvm.org/D22220
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
