[clang-tidy] Relax the way misc-move-const-arg treats trivially copyable types

[oleg]

Summary

[clang-tidy] Relax misc-move-const-arg checks w.r.t. the trivially copyable types

• it's OK to move() these as they may stop being trivially copyable in the future + this applies to user-defined types + yet that loses the checks for scalars + added an option to control this feature + added a test

[aaron.ballman]

What problem are you trying to solve with this relaxation?

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

26

I'm not keen on these regressions and would prefer to keep these tests working.

42–43

What's with the preceding x in xCHECK-MESSAGES and xCHECK-FIXES? It seems this is not being properly checked (the 'int' in the message would be failing, otherwise).

[oleg]

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

42–43

That was meant to be removed. I.E. the code generates no errors now as that is the case I am trying to solve.

[aaron.ballman]

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

42–43

Okay, that's what was causing my confusion then. :-)

I'm of two minds about this change. On the one hand, you're absolutely correct that it should be harmless to move trivially copyable types (including scalars). However, there's also utility in telling users they've written useless code that doesn't do anything.

I think this should be configured as an option that is disabled by default (so this code continues to warn), because the purpose to this check is to point out situations where the user expects a move to have impact and it's not.

[oleg]

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

42–43

Well, there are certainly two ways of looking at the existing warnings:
a) This type is trivially copyable, so a warning is useful, so let's remove the "move" as it's a copy any way. Or,
b) This type is trivially copyable today, yet that's just an implementation detail of that particular type, so I'll keep the "move" and silence the warning globally.

Obviously there is value in finding sites for (a), yet, I think, the production case would be (b).

So, if you think that both flows are required, I'll thread an option through the code. Do we have an existing way of customizing a checker? Or would I have to create a less pedantic checker?

[aaron.ballman]

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

42–43

Obviously there is value in finding sites for (a), yet, I think, the production case would be (b).

The documentation for the check specifically says: "The check warns... if std::move() is called with an argument of a trivially-copyable type, ...", so this seems like it was a design consideration made from the start. Given that the utility of the test is in finding cases where the move is a noop, I think the default should be (a), but I see the utility in providing an option for (b) so that you don't have to silence the diagnostic globally.

So, if you think that both flows are required, I'll thread an option through the code. Do we have an existing way of customizing a checker? Or would I have to create a less pedantic checker?

Check out MisplacedWideningCastCheck for an example of how to thread an option around. You should also modify the documentation to call out the new option (usually with an example) and test the option in a test file.

One nice thing about this option is: once it's provided, I'm no longer worried about the behavior of scalar types changing when the user elects to use the option.

[oleg]

Reworked the code so that it's controlled by a user-visible option.

[aaron.ballman]

LGTM aside from some small nits.

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

36 ↗

(On Diff #123216)

of the trivially -> of trivially

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

42 ↗

(On Diff #123216)

This should spell out the full diagnostic text because it's the first instance of that diagnostic in the file (subsequent instances of that text can be shortened).

[oleg]

Thank you for the review, Aaron!

What's the lifecycle of these patches? Could you commit it on my behalf please?

[aaron.ballman]

In D39863#929429, @oleg wrote:

Thank you for the review, Aaron!

What's the lifecycle of these patches? Could you commit it on my behalf please?

Normally I could commit for you, but I'm currently traveling and don't have the ability to watch the bots to see if they break for some reason.

However, if you are planning to continue development in the community, you could request commit access to commit yourself: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

Once you commit the patch, you should make sure you sit in the IRC channel (#llvm on OFTC) to see if any bots complain about the commit (most of the bots send email on a breakage, but a few do not).

[oleg]

I've just rebased the patch over the top of LLVM/Clang/tools-extra master branches and tests pass: make check-clang-tools.

Could one of the reviewers commit this please?

[aaron.ballman]

In D39863#929572, @oleg wrote:

I've just rebased the patch over the top of LLVM/Clang/tools-extra master branches and tests pass: make check-clang-tools.

Could one of the reviewers commit this please?

I can do it on Tue/Wed when I get back in town if no one gets to it before me.

[oleg]

Aaron, gentle ping...

[aaron.ballman]

Thank you for the ping, this fell off my radar (sorry about the delay in committing it) -- I've commit in r319111.

clang-tidy/misc/MoveConstantArgumentCheck.cpp

if (const CXXRecordDecl *R = Arg->getType()->getAsCXXRecordDecl()) {
       if (R->isLambda())
         return;
       // Don't warn when the type is not copyable.
       for (const auto *Ctor : R->ctors()) {
         if (Ctor->isCopyConstructor() && Ctor->isDeleted())
           return;
       }
     }
+
+    if (!IsConstArg && IsTriviallyCopyable)
+      return;
+
     bool IsVariable = isa<DeclRefExpr>(Arg);
     const auto *Var =
         IsVariable ? dyn_cast<DeclRefExpr>(Arg)->getDecl() : nullptr;
     auto Diag = diag(FileMoveRange.getBegin(),
                      "std::move of the %select{|const }0"
                      "%select{expression|variable %4}1 "
                      "%select{|of the trivially-copyable type %5 }2"
                      "has no effect; remove std::move()"

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

 // RUN: %check_clang_tidy %s misc-move-const-arg %t
 
 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) {
   return static_cast<typename std::remove_reference<_Tp>::type &&>(__t);
 }
 
+template <typename _Tp>
+constexpr _Tp &&
+forward(typename remove_reference<_Tp>::type &__t) noexcept {
+  return static_cast<_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 the trivially-copyable type 'int' 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 'x2' of the trivially-copyable type 'int'
-  // CHECK-FIXES: return x2;
-}
-
-int *f3(int *x3) {
-  return std::move(x3);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 'x3' of the trivially-copyable type 'int *'
-  // CHECK-FIXES: return x3;
+struct TriviallyCopyable {
+  int i;
+};
+
+void f(TriviallyCopyable) {}
+
+void g() {
+  TriviallyCopyable obj;
+  f(std::move(obj));
+
+  // xCHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the expression of the trivially-copyable type 'int' has no effect; remove std::move() [misc-move-const-arg]
+  // xCHECK-FIXES: return 42;
 }
 
 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 'x5' has no effect; remove std::move() or make the variable non-const [misc-move-const-arg]
   // CHECK-FIXES: return x5;
public:
   MoveOnly(MoveOnly &&other) = default;
   MoveOnly &operator=(MoveOnly &&other) = default;
 };
 template <class T>
 void Q(T);
 void moveOnlyNegatives(MoveOnly val) {
   Q(std::move(val));
 }
+
+void fmovable(MoveSemantics);
+
+void lambda1() {
+  auto f = [](MoveSemantics m) {
+    fmovable(std::move(m));
+  };
+  f(MoveSemantics());
+}
+
+template<class T> struct function {};
+
+template<typename Result, typename... Args>
+class function<Result(Args...)> {
+public:
+  function() = default;
+  void operator()(Args... args) const {
+    fmovable(std::forward<Args>(args)...);
+  }
+};
+
+void functionInvocation() {
+  function<void(MoveSemantics)> callback;
+  MoveSemantics m;
+  callback(std::move(m));
+}
+
+void lambda2() {
+  function<void(MoveSemantics)> callback;
+
+  auto f = [callback = std::move(callback)](MoveSemantics m) mutable {
+    callback(std::move(m));
+  };
+  f(MoveSemantics());
+}