[clang-tidy] misc-move-const-arg: Detect if result of std::move() is being passed as a const ref argument

[mboehme]

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.

[aaron.ballman]

clang-tidy/misc/MoveConstantArgumentCheck.cpp

20 ↗

(On Diff #60327)

Instead of an unnamed namespace, you should use static functions per our coding standards (http://llvm.org/docs/CodingStandards.html#anonymous-namespaces).

22 ↗

(On Diff #60327)

Should pass Diag by reference rather than by pointer.

26 ↗

(On Diff #60327)

Should only use auto when the type is explicitly spelled out in the initialization (which this is not). Same elsewhere.

[alexfh]

Thank you for working on this! A few more comments in addition to what Aaron has written.

clang-tidy/misc/MoveConstantArgumentCheck.cpp

22 ↗

(On Diff #60327)

I'd prefer the parameter to be named Call instead of TheCallExpr. Would make it more consistent with the rest of clang-tidy code.

37 ↗

(On Diff #60327)

Alternatively, this function could return a llvm::SmallVector<FixItHint, 2> (either empty or with the fixes) and the caller could just feed it to the result of diag(). Not much difference though, just pointing to this possibility.

42 ↗

(On Diff #60327)

With two lines of implementation and just one caller, this function doesn't seem to add any value.

116 ↗

(On Diff #60327)

Is this equivalent to just ignoring all cases where CallMove->getLocStart().isMacroID()? If not, what's the subset of the cases that would be ignored by isMacroID(), but not with the current condition?

[mboehme]

clang-tidy/misc/MoveConstantArgumentCheck.cpp

37 ↗

(On Diff #60327)

Thanks for the suggestion! If it's OK with you, though, I'd like to keep this as-is -- I feel it makes the call-site clearer.

42 ↗

(On Diff #60327)

I've inlined the function. (I think I originally had two places where it got called, so I factored it out, but then one of those places went away.)

116 ↗

(On Diff #60327)

Actually, having looked into this some more, it seems that the macro cases are already handled by the check "if (!FileMoveRange.isValid())" above -- so I'm deleting this entire block of code.

(I think I added the isMacroBodyExpansion() and isMacroArgExpansion() checks first before I moved the FileMoveRange.isValid() check, which then made them unnecessary.)

[aaron.ballman]

test/clang-tidy/misc-move-const-arg.cpp

75–76 ↗

(On Diff #60523)

This type isn't non-moveable. For that, you need to explicitly delete the move constructor. Perhaps a better name is NonMoveConstructible?

Also, no need for the public access specifier.

116 ↗

(On Diff #60523)

No need for the public access specifier.

[mboehme]

test/clang-tidy/misc-move-const-arg.cpp

75–76 ↗

(On Diff #60523)

This type isn't non-moveable. For that, you need to explicitly delete the move constructor.

Can you expand on this?

The standard says: "If the definition of a class X does not explicitly declare a move constructor, one will be implicitly declared as defaulted if and only if - X does not have a user-declared copy constructor [...]" (12.8/9).

Because I'm declaring a copy constructor, I would thus have expected not to get an implicitly-declared move constructor. Where am I going wrong here?

Perhaps a better name is NonMoveConstructible?

But the type _is_ move-constructible (in the sense of std::is_move_constructible<>).

Also, no need for the public access specifier.

Oops -- thanks for catching this.

Actually, I meant to make this a class -- not sure how the "struct" slipped in there.

116 ↗

(On Diff #60523)

Thanks for cathching this -- as above, I meant to make this a class.

[aaron.ballman]

test/clang-tidy/misc-move-const-arg.cpp

75–76 ↗

(On Diff #60671)

Can you expand on this?

The standard says: "If the definition of a class X does not explicitly declare a move constructor, one will be implicitly declared as defaulted if and only if - X does not have a user-declared copy constructor [...]" (12.8/9).

Because I'm declaring a copy constructor, I would thus have expected not to get an implicitly-declared move constructor. Where am I going wrong here?

Move operations are optimized versions of a copy operation, so a failed move operation can fall back to perform a copy operation instead and achieve the same operational semantics but with slightly worse performance. Because you have a copy constructor, you do not get an implicitly-declared move constructor (so that's correct), but that doesn't mean the type cannot be used in a context where the user expects a move (i.e., calling std::move() on it) -- the operation just falls back on the copy constructor.

Basically, there's an operational difference between not having the implicitly-declared move constructor and having a deleted move constructor. In the former, fallback to copy happens and in the latter you get a diagnostic. So when I hear "non-moveable type", my brain assumes you mean "get a diagnostic when you try to move."

[mboehme]

test/clang-tidy/misc-move-const-arg.cpp

75–76 ↗

(On Diff #60671)

Ah, now I see what you mean.

I've renamed "NonMoveable" to "NoMoveSemantics" (and "Moveable" to "MoveSemantics" for consistency). What do you think?

(I don't like NonMoveConstructible because it's prone to the same ambiguity -- or even more so, because std::is_move_constructible<> uses exactly the same term and the type is move constructible in the sense of that trait.)

[aaron.ballman]

LGTM, but you should wait for @alexfh to sign off as well since he had some comments.

[alexfh]

LG. Thanks!

[alexfh]

BTW, I'll commit the patch for you.

clang-tools-extra/trunk/clang-tidy/misc/MoveConstantArgumentCheck.cpp

 #include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;
 
 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),
+          .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"
                      "%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());
+    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");
 
-    if (BeforeArgumentsRange.isValid() && AfterArgumentsRange.isValid()) {
-      Diag << FixItHint::CreateRemoval(BeforeArgumentsRange)
-           << FixItHint::CreateRemoval(AfterArgumentsRange);
-    }
+    ReplaceCallWithArg(CallMove, Diag, SM, getLangOpts());
   }
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/docs/clang-tidy/checks/misc-move-const-arg.rst

 .. title:: clang-tidy - misc-move-const-arg
 
 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

clang-tools-extra/trunk/test/clang-tidy/misc-move-const-arg.cpp

template <typename T> T f10(const int x10) {
   return std::move(x10);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the const variable
   // CHECK-FIXES: return x10;
 }
 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); });
+}