[clang-tools-extra] r272896 - [clang-tidy] misc-move-const-arg: Detect if result of std::move() is being passed as a const ref argument
Alexander Kornienko via cfe-commits
cfe-commits at lists.llvm.org
Thu Jun 16 07:32:42 PDT 2016
Author: alexfh
Date: Thu Jun 16 09:32:41 2016
New Revision: 272896
URL: http://llvm.org/viewvc/llvm-project?rev=272896&view=rev
Log:
[clang-tidy] misc-move-const-arg: Detect if result of std::move() is being passed as a const ref argument
Summary:
Conceptually, this is very close to the existing functionality of misc-move-const-arg, which is why I'm adding it here and not creating a new check. For example, for a type A that is both movable and copyable, this
const A a1;
A a2(std::move(a1));
is not only a case where a const argument is being passed to std::move(), but the result of std::move() is also being passed as a const reference (due to overload resolution).
The new check typically triggers (exclusively) in cases where people think they're dealing with a movable type, but in fact the type is not movable.
Reviewers: hokein, aaron.ballman, alexfh
Subscribers: aaron.ballman, cfe-commits
Patch by Martin Boehme!
Differential Revision: http://reviews.llvm.org/D21223
Modified:
clang-tools-extra/trunk/clang-tidy/misc/MoveConstantArgumentCheck.cpp
clang-tools-extra/trunk/docs/clang-tidy/checks/misc-move-const-arg.rst
clang-tools-extra/trunk/test/clang-tidy/misc-move-const-arg.cpp
Modified: clang-tools-extra/trunk/clang-tidy/misc/MoveConstantArgumentCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/MoveConstantArgumentCheck.cpp?rev=272896&r1=272895&r2=272896&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/misc/MoveConstantArgumentCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/misc/MoveConstantArgumentCheck.cpp Thu Jun 16 09:32:41 2016
@@ -17,30 +17,62 @@ namespace clang {
namespace tidy {
namespace misc {
+static void ReplaceCallWithArg(const CallExpr *Call, DiagnosticBuilder &Diag,
+ const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ const Expr *Arg = Call->getArg(0);
+
+ CharSourceRange BeforeArgumentsRange = Lexer::makeFileCharRange(
+ CharSourceRange::getCharRange(Call->getLocStart(), Arg->getLocStart()),
+ SM, LangOpts);
+ CharSourceRange AfterArgumentsRange = Lexer::makeFileCharRange(
+ CharSourceRange::getCharRange(Call->getLocEnd(),
+ Call->getLocEnd().getLocWithOffset(1)),
+ SM, LangOpts);
+
+ if (BeforeArgumentsRange.isValid() && AfterArgumentsRange.isValid()) {
+ Diag << FixItHint::CreateRemoval(BeforeArgumentsRange)
+ << FixItHint::CreateRemoval(AfterArgumentsRange);
+ }
+}
+
void MoveConstantArgumentCheck::registerMatchers(MatchFinder *Finder) {
if (!getLangOpts().CPlusPlus)
return;
- Finder->addMatcher(callExpr(callee(functionDecl(hasName("::std::move"))),
- argumentCountIs(1),
- unless(isInTemplateInstantiation()))
- .bind("call-move"),
+
+ auto MoveCallMatcher =
+ callExpr(callee(functionDecl(hasName("::std::move"))), argumentCountIs(1),
+ unless(isInTemplateInstantiation()))
+ .bind("call-move");
+
+ Finder->addMatcher(MoveCallMatcher, this);
+
+ auto ConstParamMatcher = forEachArgumentWithParam(
+ MoveCallMatcher, parmVarDecl(hasType(references(isConstQualified()))));
+
+ Finder->addMatcher(callExpr(ConstParamMatcher).bind("receiving-expr"), this);
+ Finder->addMatcher(cxxConstructExpr(ConstParamMatcher).bind("receiving-expr"),
this);
}
void MoveConstantArgumentCheck::check(const MatchFinder::MatchResult &Result) {
const auto *CallMove = Result.Nodes.getNodeAs<CallExpr>("call-move");
+ const auto *ReceivingExpr = Result.Nodes.getNodeAs<Expr>("receiving-expr");
const Expr *Arg = CallMove->getArg(0);
SourceManager &SM = Result.Context->getSourceManager();
+ CharSourceRange MoveRange =
+ CharSourceRange::getCharRange(CallMove->getSourceRange());
+ CharSourceRange FileMoveRange =
+ Lexer::makeFileCharRange(MoveRange, SM, getLangOpts());
+ if (!FileMoveRange.isValid())
+ return;
+
bool IsConstArg = Arg->getType().isConstQualified();
bool IsTriviallyCopyable =
Arg->getType().isTriviallyCopyableType(*Result.Context);
if (IsConstArg || IsTriviallyCopyable) {
- auto MoveRange = CharSourceRange::getCharRange(CallMove->getSourceRange());
- auto FileMoveRange = Lexer::makeFileCharRange(MoveRange, SM, getLangOpts());
- if (!FileMoveRange.isValid())
- return;
bool IsVariable = isa<DeclRefExpr>(Arg);
auto Diag = diag(FileMoveRange.getBegin(),
"std::move of the %select{|const }0"
@@ -49,19 +81,13 @@ void MoveConstantArgumentCheck::check(co
"has no effect; remove std::move()")
<< IsConstArg << IsVariable << IsTriviallyCopyable;
- auto BeforeArgumentsRange = Lexer::makeFileCharRange(
- CharSourceRange::getCharRange(CallMove->getLocStart(),
- Arg->getLocStart()),
- SM, getLangOpts());
- auto AfterArgumentsRange = Lexer::makeFileCharRange(
- CharSourceRange::getCharRange(
- CallMove->getLocEnd(), CallMove->getLocEnd().getLocWithOffset(1)),
- SM, getLangOpts());
-
- if (BeforeArgumentsRange.isValid() && AfterArgumentsRange.isValid()) {
- Diag << FixItHint::CreateRemoval(BeforeArgumentsRange)
- << FixItHint::CreateRemoval(AfterArgumentsRange);
- }
+ ReplaceCallWithArg(CallMove, Diag, SM, getLangOpts());
+ } else if (ReceivingExpr) {
+ auto Diag = diag(FileMoveRange.getBegin(),
+ "passing result of std::move() as a const reference "
+ "argument; no move will actually happen");
+
+ ReplaceCallWithArg(CallMove, Diag, SM, getLangOpts());
}
}
Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/misc-move-const-arg.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/misc-move-const-arg.rst?rev=272896&r1=272895&r2=272896&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/misc-move-const-arg.rst (original)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/misc-move-const-arg.rst Thu Jun 16 09:32:41 2016
@@ -3,8 +3,17 @@
misc-move-const-arg
===================
-The check warns if ``std::move()`` is called with a constant argument or an
-argument of a trivially-copyable type, e.g.:
+The check warns
+
+ - if ``std::move()`` is called with a constant argument,
+ - if ``std::move()`` is called with an argument of a trivially-copyable type,
+ or
+ - if the result of ``std::move()`` is passed as a const reference argument.
+
+In all three cases, the check will suggest a fix that removes the
+``std::move()``.
+
+Here are examples of each of the three cases:
.. code:: c++
@@ -13,3 +22,7 @@ argument of a trivially-copyable type, e
int x;
return std::move(x); // Warning: std::move of the variable of a trivially-copyable type has no effect
+
+ void f(const string &s);
+ string s;
+ f(std::move(s)); // Warning: passing result of std::move as a const reference argument; no move will actually happen
Modified: clang-tools-extra/trunk/test/clang-tidy/misc-move-const-arg.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/misc-move-const-arg.cpp?rev=272896&r1=272895&r2=272896&view=diff
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/misc-move-const-arg.cpp (original)
+++ clang-tools-extra/trunk/test/clang-tidy/misc-move-const-arg.cpp Thu Jun 16 09:32:41 2016
@@ -71,3 +71,90 @@ void f11() {
f10<int>(1);
f10<double>(1);
}
+
+class NoMoveSemantics {
+ public:
+ NoMoveSemantics();
+ NoMoveSemantics(const NoMoveSemantics &);
+
+ NoMoveSemantics &operator=(const NoMoveSemantics &);
+};
+
+void callByConstRef(const NoMoveSemantics &);
+void callByConstRef(int i, const NoMoveSemantics &);
+
+void moveToConstReferencePositives() {
+ NoMoveSemantics obj;
+
+ // Basic case.
+ callByConstRef(std::move(obj));
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: passing result of std::move() as
+ // CHECK-FIXES: callByConstRef(obj);
+
+ // Also works for second argument.
+ callByConstRef(1, std::move(obj));
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: passing result of std::move() as
+ // CHECK-FIXES: callByConstRef(1, obj);
+
+ // Works if std::move() applied to a temporary.
+ callByConstRef(std::move(NoMoveSemantics()));
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: passing result of std::move() as
+ // CHECK-FIXES: callByConstRef(NoMoveSemantics());
+
+ // Works if calling a copy constructor.
+ NoMoveSemantics other(std::move(obj));
+ // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: passing result of std::move() as
+ // CHECK-FIXES: NoMoveSemantics other(obj);
+
+ // Works if calling assignment operator.
+ other = std::move(obj);
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: passing result of std::move() as
+ // CHECK-FIXES: other = obj;
+}
+
+class MoveSemantics {
+ public:
+ MoveSemantics();
+ MoveSemantics(MoveSemantics &&);
+
+ MoveSemantics &operator=(MoveSemantics &&);
+};
+
+void callByValue(MoveSemantics);
+
+void callByRValueRef(MoveSemantics &&);
+
+template <class T>
+void templateFunction(T obj) {
+ T other = std::move(obj);
+}
+
+#define M3(T, obj) \
+ do { \
+ T other = std::move(obj); \
+ } while (true)
+
+#define CALL(func) (func)()
+
+void moveToConstReferenceNegatives() {
+ // No warning when actual move takes place.
+ MoveSemantics move_semantics;
+ callByValue(std::move(move_semantics));
+ callByRValueRef(std::move(move_semantics));
+ MoveSemantics other(std::move(move_semantics));
+ other = std::move(move_semantics);
+
+ // No warning if std::move() not used.
+ NoMoveSemantics no_move_semantics;
+ callByConstRef(no_move_semantics);
+
+ // No warning if instantiating a template.
+ templateFunction(no_move_semantics);
+
+ // No warning inside of macro expansions.
+ M3(NoMoveSemantics, no_move_semantics);
+
+ // No warning inside of macro expansion, even if the macro expansion is inside
+ // a lambda that is, in turn, an argument to a macro.
+ CALL([no_move_semantics] { M3(NoMoveSemantics, no_move_semantics); });
+}
More information about the cfe-commits
mailing list