mboehme created this revision.
mboehme added reviewers: alexfh, hokein.
mboehme added a subscriber: cfe-commits.
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.
http://reviews.llvm.org/D21223
Files:
clang-tidy/misc/MoveConstantArgumentCheck.cpp
docs/clang-tidy/checks/misc-move-const-arg.rst
test/clang-tidy/misc-move-const-arg.cpp
Index: test/clang-tidy/misc-move-const-arg.cpp
===================================================================
--- test/clang-tidy/misc-move-const-arg.cpp
+++ test/clang-tidy/misc-move-const-arg.cpp
@@ -71,3 +71,90 @@
f10<int>(1);
f10<double>(1);
}
+
+struct NonMoveable {
+ public:
+ NonMoveable();
+ NonMoveable(const NonMoveable &);
+
+ NonMoveable &operator=(const NonMoveable &);
+};
+
+void callByConstRef(const NonMoveable &);
+void callByConstRef(int i, const NonMoveable &);
+
+void moveToConstReferencePositives() {
+ NonMoveable 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(NonMoveable()));
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: passing result of std::move() as
+ // CHECK-FIXES: callByConstRef(NonMoveable());
+
+ // Works if calling a copy constructor.
+ NonMoveable other(std::move(obj));
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: passing result of std::move() as
+ // CHECK-FIXES: NonMoveable 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;
+}
+
+struct Moveable {
+ public:
+ Moveable();
+ Moveable(Moveable &&);
+
+ Moveable &operator=(Moveable &&);
+};
+
+void callByValue(Moveable);
+
+void callByRValueRef(Moveable &&);
+
+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.
+ Moveable moveable;
+ callByValue(std::move(moveable));
+ callByRValueRef(std::move(moveable));
+ Moveable other(std::move(moveable));
+ other = std::move(moveable);
+
+ // No warning if std::move() not used.
+ NonMoveable non_moveable;
+ callByConstRef(non_moveable);
+
+ // No warning if instantiating a template.
+ templateFunction(non_moveable);
+
+ // No warning inside of macro expansions.
+ M3(NonMoveable, non_moveable);
+
+ // 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([non_moveable] { M3(NonMoveable, non_moveable); });
+}
Index: docs/clang-tidy/checks/misc-move-const-arg.rst
===================================================================
--- docs/clang-tidy/checks/misc-move-const-arg.rst
+++ docs/clang-tidy/checks/misc-move-const-arg.rst
@@ -3,13 +3,26 @@
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++
const string s;
return std::move(s); // Warning: std::move of the const variable has no effect
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
Index: clang-tidy/misc/MoveConstantArgumentCheck.cpp
===================================================================
--- clang-tidy/misc/MoveConstantArgumentCheck.cpp
+++ clang-tidy/misc/MoveConstantArgumentCheck.cpp
@@ -17,51 +17,112 @@
namespace tidy {
namespace misc {
+namespace {
+
+void ReplaceCallWithArg(const CallExpr *TheCallExpr, DiagnosticBuilder *Diag,
+ const SourceManager &SM, const LangOptions &LangOpts) {
+ const Expr *Arg = TheCallExpr->getArg(0);
+
+ auto BeforeArgumentsRange = Lexer::makeFileCharRange(
+ CharSourceRange::getCharRange(TheCallExpr->getLocStart(),
+ Arg->getLocStart()),
+ SM, LangOpts);
+ auto AfterArgumentsRange = Lexer::makeFileCharRange(
+ CharSourceRange::getCharRange(
+ TheCallExpr->getLocEnd(),
+ TheCallExpr->getLocEnd().getLocWithOffset(1)),
+ SM, LangOpts);
+
+ if (BeforeArgumentsRange.isValid() && AfterArgumentsRange.isValid()) {
+ (*Diag) << FixItHint::CreateRemoval(BeforeArgumentsRange)
+ << FixItHint::CreateRemoval(AfterArgumentsRange);
+ }
+}
+
+CharSourceRange FileCharRangeForExpr(const Expr *TheExpr,
+ const SourceManager &SM,
+ const LangOptions &LangOpts) {
+ auto CharRange = CharSourceRange::getCharRange(TheExpr->getSourceRange());
+ return Lexer::makeFileCharRange(CharRange, SM, LangOpts);
+}
+
+} // namespace
+
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();
+ auto FileMoveRange = FileCharRangeForExpr(CallMove, 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"
"%select{expression|variable}1 "
"%select{|of a trivially-copyable type }2"
"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) {
+ // Ignore macro body and argument expansions.
+ //
+ // Macro argument expansions need to be ignored to handle this case
+ // correctly:
+ //
+ // CALL([non_moveable] {
+ // MACRO(NonMoveable, non_moveable);
+ // });
+ //
+ // where
+ //
+ // #define MACRO(T, obj) do { \
+ // T other = std::move(obj); \
+ // } while(true)
+ //
+ // #define CALL(func) (func)()
+ //
+ // Here, the std::move() is part of a macro expansion, but
+ // isMacroBodyExpansion() nevertheless returns false because that macro
+ // expansion is inside a lambda which, in turn, is an argument to a macro --
+ // and that appears to take precendence.
+ if (SM.isMacroBodyExpansion(CallMove->getLocStart()) ||
+ SM.isMacroArgExpansion(CallMove->getLocStart())) {
+ return;
}
+
+ 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());
}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
