Const std::move() argument ClangTidy check

[dvadym]

Summary

ClangTidy check for finding cases when std::move() is called with const or trivially copyable arguments, that doesn't lead to any move or argument but it makes copy. FixIt generates patch for removing call of std::move().

[alexfh]

Thank you for tackling this!

A high-level comment: the check needs to be somewhat more general. Const-qualified variables are just a specific case of an rvalue. The check should warn on all usages of std::move with an rvalue argument (except in templates with arguments of dependent types).

Additionally, I would extend the check (arguably, this should be a separate check) to complain about use of std::move with trivially-copyable types, as it seems that there's no reason to prefer moving for these types anyway (again, with the exception of dependent types in templates).

clang-tidy/misc/MiscTidyModule.cpp

70 ↗

(On Diff #32149)

The name should start with "misc-". Please also update the position of the statement to maintain sorting.

clang-tidy/misc/MoveConstantArgumentCheck.cpp

3 ↗

(On Diff #32149)

I suppose this is not needed. The line below as well.

18 ↗

(On Diff #32149)

Please follow LLVM naming convention (http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly). s/result/Result/

24 ↗

(On Diff #32149)

s/SourceManager* sm/SourceManager &SM/

26 ↗

(On Diff #32149)

No need for "clang::" as the code is inside the clang namespace.

31 ↗

(On Diff #32149)

The message could be more helpful. For example, "std::move of the const variable '%0' doesn't have effect". We could also add a recommendation on how to fix that (e.g. "remove std::move() or make the variable non-const").

Also, in case it's not a variable (DeclRefExpr), the warning shouldn't say "variable".

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

29 ↗

(On Diff #32149)

That also doesn't look like a reasonable use of std::move. We should probably extend this check to flag std::move applied to rvalues (in general, not only usages of const variables), scalar types, pointer types and probably also all other trivially-copyable types (I don't immediately see why moving those could ever be better than copying). These warnings shouldn't trigger for dependent types and in template instantiations.

[alexfh]

A few more comments.

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

1 ↗

(On Diff #32149)

Please use check_clang_tidy.py instead:

// RUN: %python %S/check_clang_tidy.py %s move-const-arg %t

and remove // REQUIRES: shell, as it's not needed any more.

41 ↗

(On Diff #32149)

Please make the checked code lines unique to avoid matching in a wrong place. FileCheck (the utility that is called by the check_clang_tidy.py script to verify the // CHECK-... lines, http://llvm.org/docs/CommandGuide/FileCheck.html) doesn't take the location of the // CHECK-FIXES: line in the test file into consideration, it just verifies that the fixed file has a subsequence of lines that matches all // CHECK-FIXES lines in that order. Here, for example, return x; would equally match, if the check would fix line 34 instead of line 39. We could solve this by numbering lines so that CHECK-FIXES patterns could refer to the line numbers, but until we do that (if we decide so), making the checked lines unique is the way to go (e.g. by using different variable names in each test case instead of just x).

[sbenza]

clang-tidy/misc/MoveConstantArgumentCheck.cpp

20 ↗

(On Diff #32149)

You can move both checks to the matcher.
Something like:

callExpr(callee(functionDecl(hasName("::std::move"))),
         argumentCountIs(1),
         hasArgument(0, expr(hasType(isConstQualified()))))

32 ↗

(On Diff #32149)

It is better to remove/insert than to replace.
Replacement could strip comments inadvertently.
In this case it also removes the need to find the text for the replacement.
You could do:

Remove(CallMove->Start(), Arg->Start())
Remove(CallMove->End(), CallMove->End())

[alexfh]

Vadym, what's the state of this patch?

[dvadym]

1.Most comments addressed
2.Taking into consideration applying move to trivially copyable objects
3.Different message if move argument variable or expression.

It's not addressed yet case when move argument is depend in some way on template argument. There is comment in code about this.

[dvadym]

Small clean-up

[dvadym]

Thanks alexfh and sbenza for comments!
I've addressed most of them. Could you please advice how to find that some expression has type which is template dependent?

Regards,
Vadym

clang-tidy/misc/MoveConstantArgumentCheck.cpp

20 ↗

(On Diff #32149)

Thanks, according to alexfh comments I've decided to apply this check not only for const but also for trivially copyable types.

31 ↗

(On Diff #32149)

Thanks, I've addressed your comments.

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

29 ↗

(On Diff #32149)

Thanks, I've implemented for trivially copyable types. But I didn't find how to find dependent types: Arg->isTypeDependent(), Arg->isInstantiationDependent(), Arg->getType()->isDependentType() doesn't look like solving this problem. Could you please advice?

41 ↗

(On Diff #32149)

Thanks, I've set different names to variables

[aaron.ballman]

clang-tidy/misc/MoveConstantArgumentCheck.cpp

1 ↗

(On Diff #39080)

Missing new file legal text.

9 ↗

(On Diff #39080)

Formatting -- the * should go with Finder. May want to run clang-format over the entire patch; there's a lot of other formatting issues I will refrain from commenting on.

10 ↗

(On Diff #39080)

Please only register this matcher for C++ code.

22 ↗

(On Diff #39080)

Arg->getType()->isDependentType() should do what you want, if I understand you properly.

29 ↗

(On Diff #39080)

isa<DeclRefExpr> instead of dyn_cast and check against nullptr.

34 ↗

(On Diff #39080)

This line reads a bit strangely to me. Perhaps "has no effect" instead? Also, our diagnostics are not full sentences, so you should remove the period, and instead do: "; remove std::move()"

clang-tidy/misc/MoveConstantArgumentCheck.h

1 ↗

(On Diff #39080)

Missing header include guards and header legal text.

[alexfh]

clang-tidy/misc/MoveConstantArgumentCheck.cpp

22 ↗

(On Diff #39080)

Yep, should be what you need.

30 ↗

(On Diff #39080)

Please don't string += as a way to build messages. This creates a temporary each time and reallocates the string buffer. Use one of the these:

std::string message = (llvm::Twine("...") + "..." + "...").str()

(only in a single expression, i.e. don't create variables of the llvm::Twine type, as this can lead to dangling string references), or:

std::string buffer;
llvm::raw_string_ostream message(buffer);
message << "...";
message << "...";
// then use message.str() where you would use an std::string.

The second alternative would be more suitable for this kind of code.

BUT, even this is not needed in the specific case of producing diagnostics, as clang provides a powerful template substitution mechanism, and your code could be written more effectively:

diag("std::move of the %select{const |}0 %select{variable|expression}1 ...") << IsConstArg << IsVariable << ...;

39 ↗

(On Diff #39080)

The variable names don't add much value here. Either remove the variables and use the expressions below or give the variables more useful names. Also note that SourceRange can be constructed from a single token (SourceRange(CallMove->getLocEnd())).

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

1 ↗

(On Diff #39080)

This has changed once more, now %check_clang_tidy should be used instead of the script itself. Please also rebase the patch on top of current HEAD.

4 ↗

(On Diff #39080)

The comment is not useful here. This is a mock implementation, and it's not important where does this come from. Please also clang-format the test with -style=file (to pick up formatting options specific to tests, namely unlimited columns limit).

30 ↗

(On Diff #39080)

How did you find that Arg->getType()->isDependentType() doesn't work?

[alexfh]

clang-tidy/misc/MoveConstantArgumentCheck.cpp

30 ↗

(On Diff #39080)

This should read:
"Please don't use string += as a way to build messages. This creates a temporary each time and reallocates the string buffer. Instead, use one of these patterns:"

[dvadym]

Addressed reviewers' comments
1.Change message generation on using diag() string substitution
2.Added copyright
3.Rebase

[dvadym]

Thanks for review comments! Could you please have an another look and help me with my questions?

clang-tidy/misc/MoveConstantArgumentCheck.cpp

40 ↗

(On Diff #39717)

Could you please advice how can I correctly make removal?
I expected that
FixItHint::CreateRemoval(SourceRange(CallMove->getLocStart(), Arg->getLocStart())) removes "std::move(" but it removes "std::move(varname", so from a "move" call only ")" is left

10 ↗

(On Diff #39080)

I didn't find how it can be done, could you please advice?

22 ↗

(On Diff #39080)

It doesn't do what it's needed. See for example function f6, f7, f8 in tests. ::check method is called once on any instantion of f6, Arg->getType()->isDependentType() returns false, so the check returns 2 warning.

29 ↗

(On Diff #39080)

Thanks

30 ↗

(On Diff #39080)

Thanks, it made code much clearer

[aaron.ballman]

clang-tidy/misc/MoveConstantArgumentCheck.cpp

11 ↗

(On Diff #39717)

I didn't find how it can be done, could you please advice?

This is the usual way we do it (in the registerMatchers() function):

if (!getLangOpts().CPlusPlus)
  return;

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

1 ↗

(On Diff #39717)

Please run clang-format over the test files as well.

[alexfh]

clang-tidy/misc/MoveConstantArgumentCheck.cpp

23 ↗

(On Diff #39717)

The problem is that each template class or function can have several representations in the AST: one for the template definition and one for each instantiation. Usually, we don't need to even look at the instantiations, when we want to reason about the code in the general case. You can filter out expressions belonging to template instantiations using this narrowing matcher: unless(isInTemplateInstantiation()). And for template definitions the type will be marked as dependent.

31 ↗

(On Diff #39717)

The variable here is only used once and its name doesn't make the code much clearer compared to the expression itself. Please remove it.

40 ↗

(On Diff #39717)

FixItHint::CreateRemoval and many other methods accept CharSourceRange instead of SourceRange. The former is a SourceRange + a flag telling whether the range should be treated as a character range or a token range. By default, SourceRange is converted to a CharSourceRange marked as a token range. So your current code creates a FixItHint that removes tokens from std to varname inclusive. If you want to delete everything from std to just before varname, you can create a character range from CallMove->getLocStart() to Arg->getLocStart().getLocWithOffset(-1).

However, when there's something between std::move( and varname (whitespace and/or comment(s)), might want to delete just std::move(. In this case you can take CallMove->getCallee()' (which will correspond to std::move`), and then find the first '(' token after it's end location. It's probably a rare case though, so let's go for the simpler solution for now (with getLocWithOffset and character ranges).

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

1 ↗

(On Diff #39717)

Did you forget to include the test file to the latest patch? The formatting is still off and the messages don't seem to correspond to the spelling in the code. If you have troubles with clang-format breaking the CHECK lines, be sure to use clang-format -style=file so that the test-specific configuration file is used.

[dvadym]

New in this upload:
1.Adding checking that language is C++
2.Not consider calls of std::move in template instantation
3.Creating CharSourceRange for removal
4.Using Lexer::makeFileCharRange( for processing move call in macros

[dvadym]

Thanks for comments! PTAL
Since it's added checking of trivially copyable arguments of move, it's needed to rename this check, what do you think about MoveNoEffectCheck?

clang-tidy/misc/MoveConstantArgumentCheck.cpp

11 ↗

(On Diff #39717)

Thanks

23 ↗

(On Diff #39717)

Great, thank you. It works

40 ↗

(On Diff #39717)

Thanks, creating CharSourceRange makes the trick. Also as we talked offline I've added a call of Lexer::makeFileCharRange( for processing move call in macros. Please have a look.

[alexfh]

Thank you for the update! Looks almost right. A few minor comments.

clang-tidy/misc/MoveConstantArgumentCheck.cpp

34 ↗

(On Diff #40751)

Looks like you're using Context once and all the other times you need Context->getSourceManager(). I suggest pulling it to a local variable (SourceManager &SM or SourceManager &Sources - whatever you like more) and replace the usage of Context with Result.Context.

40 ↗

(On Diff #40751)

You should be able to use CharSourceRange::getCharRange(CallMove->getSourceRange()) instead. It's slightly more compact.

45 ↗

(On Diff #40751)

Remove the comment, it's now wrong.

51 ↗

(On Diff #40751)

nit: clang-format has done a poor job here: spaces are not the best places to break this string literal. Please manually reformat this literal so that the %select{...}N constructs are not broken. Thanks!

54 ↗

(On Diff #40751)

Diagnostic messages are not full sentences and thus require neither capitalization (already fine here) nor trailing period.

56 ↗

(On Diff #40751)

After some thinking, there may be cases where the range of the whole expression can be translated to a file char range, but a sub-range of it can't. So you need to check the validity of the results of both makeFileCharRange calls in this expression before creating a fixit hint with the resulting ranges.

Also, it seems reasonable to issue a warning in any case, and fix-it hints only when we are rather certain that we can apply them safely (which the validity of all makeFileCharRange results should tell us about).

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

26 ↗

(On Diff #40751)

I see that clang-format did a wrong thing here. It should have used the tools/clang/tools/extra/test/.clang-format configuration file containing the ColumnLimit: 0 setting, which disables column limit enforcement. Please merge the CHECK-MESSAGES lines (otherwise, the second line is just ignored) and in the future use clang-format -style=file when reformatting test files.

Please also specify the full diagnostic message once including the [check-name]. The rest messages can be truncated to remove duplicating text (e.g. remove the has no effect; remove std::move(). substring).

54 ↗

(On Diff #40751)

Please add a test that insures that correct fixes are applied to template functions and only once, e.g. like this:

template <typename T> T f(const int x) {
  return std::move(x);
  // CHECK-FIXES: return x;
}
void g() {
 f<int>(1);
 f<double>(1);
}

[dvadym]

In this patch alexfh comments addressed

[dvadym]

Thanks alexfh! I've addressed your comments and uploaded new patch. PTAL

clang-tidy/misc/MoveConstantArgumentCheck.cpp

56 ↗

(On Diff #40751)

I've changed: now BeforeArgumentsRange and AfterArgumentsRange are calculated if they are valid then Removal is created. But probably it's better to write a test when some of these ranges are valid, could you please advice when it can be?

[alexfh]

Awesome! Thank you for the new check!

There are a couple of nits, but I'll fix these before submitting the patch.

clang-tidy/misc/MoveConstantArgumentCheck.cpp

42 ↗

(On Diff #41061)

s/auto/CharSourceRange/

47 ↗

(On Diff #41061)

I'd replace DiagnosticBuilder DB with auto Diag - this is more a local convention, not a general rule.

57 ↗

(On Diff #41061)

This can probably happen when Arg comes from another macro (or a macro argument). A test can be added later.

[alexfh]

Missed a couple of comments. Anyway, I'm fixing these myself as a part of commit.

clang-tidy/misc/MoveConstantArgumentCheck.cpp

42 ↗

(On Diff #41061)

... or do the reverse below on lines 54, 58.

43 ↗

(On Diff #41061)

It's not common for LLVM style to put braces around single-line bodies of loops and conditional statements.

clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
   AssignOperatorSignatureCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   InaccurateEraseCheck.cpp
   InefficientAlgorithmCheck.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MiscTidyModule.cpp
+  MoveConstantArgumentCheck.cpp 
   MoveConstructorInitCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NoexceptMoveConstructorCheck.cpp
   NonCopyableObjects.cpp
   SizeofContainerCheck.cpp
   StaticAssertCheck.cpp
   SwappedArgumentsCheck.cpp
   ThrowByValueCatchByReferenceCheck.cpp

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

 #include "ArgumentCommentCheck.h"
 #include "AssertSideEffectCheck.h"
 #include "AssignOperatorSignatureCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "InefficientAlgorithmCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
+#include "MoveConstantArgumentCheck.h"
 #include "MoveConstructorInitCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "NonCopyableObjects.h"
 #include "SizeofContainerCheck.h"
 #include "StaticAssertCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<InaccurateEraseCheck>(
         "misc-inaccurate-erase");
     CheckFactories.registerCheck<InefficientAlgorithmCheck>(
         "misc-inefficient-algorithm");
     CheckFactories.registerCheck<MacroParenthesesCheck>(
         "misc-macro-parentheses");
     CheckFactories.registerCheck<MacroRepeatedSideEffectsCheck>(
         "misc-macro-repeated-side-effects");
+    CheckFactories.registerCheck<MoveConstantArgumentCheck>(
+        "misc-move-const-arg");
     CheckFactories.registerCheck<MoveConstructorInitCheck>(
         "misc-move-constructor-init");
     CheckFactories.registerCheck<NewDeleteOverloadsCheck>(
         "misc-new-delete-overloads");
     CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
         "misc-noexcept-move-constructor");
     CheckFactories.registerCheck<NonCopyableObjectsCheck>(
         "misc-non-copyable-objects");

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

+//===--- MoveConstandArgumentCheck.h - clang-tidy -------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONTANTARGUMENTCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONTANTARGUMENTCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+class MoveConstantArgumentCheck : public ClangTidyCheck {
+public:
+  MoveConstantArgumentCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MOVECONTANTARGUMENTCHECK_H

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

+//===--- MoveConstandArgumentCheck.cpp - clang-tidy -----------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "MoveConstantArgumentCheck.h"
+
+#include <clang/Lex/Lexer.h>
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+using namespace ast_matchers;
+
+void MoveConstantArgumentCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+  Finder->addMatcher(callExpr(unless(isInTemplateInstantiation()),
+                              callee(functionDecl(hasName("::std::move"))))
+                         .bind("call-move"),
+                     this);
+}
+
+void MoveConstantArgumentCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *CallMove = Result.Nodes.getNodeAs<CallExpr>("call-move");
+  if (CallMove->getNumArgs() != 1)
+    return;
+  const Expr *Arg = CallMove->getArg(0);
+  SourceManager &SM = Result.Context->getSourceManager();
+
+  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 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()) {
+      DB << FixItHint::CreateRemoval(BeforeArgumentsRange)
+         << FixItHint::CreateRemoval(AfterArgumentsRange);
+    }
+  }
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

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

+// RUN: %check_clang_tidy %s misc-move-const-arg %t -- -- -std=c++11
+
+namespace std {
+template <typename> struct remove_reference;
+
+template <typename _Tp> struct remove_reference { typedef _Tp type; };
+
+template <typename _Tp> struct remove_reference<_Tp &> { typedef _Tp type; };
+
+template <typename _Tp> struct remove_reference<_Tp &&> { typedef _Tp type; };
+
+template <typename _Tp>
+constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t);
+
+} // namespace std
+
+class A {
+public:
+  A() {}
+  A(const A &rhs) {}
+  A(A &&rhs) {}
+};
+
+int f1() {
+  return std::move(42);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the expression of trivially-copyable type has no effect; remove std::move() [misc-move-const-arg]
+  // CHECK-FIXES: return 42;
+}
+
+int f2(int x2) {
+  return std::move(x2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable of trivially-copyable type
+  // CHECK-FIXES: return x2;
+}
+
+int *f3(int *x3) {
+  return std::move(x3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable of trivially-copyable type
+  // CHECK-FIXES: return x3;
+}
+
+A f4(A x4) { return std::move(x4); }
+
+A f5(const A x5) {
+  return std::move(x5);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the const variable
+  // CHECK-FIXES: return x5;
+}
+
+template <typename T> T f6(const T x6) { return std::move(x6); }
+
+void f7() { int a = f6(10); }
+
+#define M1(x) x
+void f8() {
+  const A a;
+  M1(A b = std::move(a);)
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: std::move of the const variable
+  // CHECK-FIXES: M1(A b = a;)
+}
+
+#define M2(x) std::move(x)
+int f9() { return M2(1); }
+
+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);
+}