[PATCH] clang-tidy: Bug 27731 - modernize-pass-by-value suggest using std::move for types that perform copies on move

[madsravn]

Summary

This is a patch for bug: https://llvm.org/bugs/show_bug.cgi?id=27731

I have excluded types which are trivially copyable from being called with std::move in the modernizer. In the current state another modernizer will catch them and change them back, thus creating a cyclic behaviour.

[alexfh]

LG. Do you need me to submit the patch for you?

Next time please create diff with the full context (http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).

Thank you!

[madsravn]

Just curious, as I'm sort of new to this. How long will it take before its merged in?

[Prazek]

You have to push it by yourself. It's ain't fun if someone do it for you :D
You have to obtain commit access http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access and then push it (you can also find in docs how to do it)

Thanks for fixing this bug, it was pissing me off.

[madsravn]

@Prazek thanks. I will look into it :)

[madsravn]

Code committed.

[alexfh]

In D20365#436335, @madsravn wrote:

Just curious, as I'm sort of new to this. How long will it take before its merged in?

I was waiting for an answer to the "Do you need me to submit the patch for you?" question. Apparently, the answer is "no" ;)

[Prazek]

Did you revert the commit? I see that it is commieted, but after it I see revert.
Also please stick to convention of commit messages http://llvm.org/docs/DeveloperPolicy.html#commit-messages
Commit message like "[clang-tidy] modernize-pass-by-value bugfix" would be much better.

[madsravn]

@alexfh I don't know how I could miss that. But I got my commit access and committed the code myself. Thanks though.

@Prazek Yes I reverted. The code made the build server (as seen on IRC) fail. So I quickly reverted. I'm gonna fix the code tonight - I had to "make clean" my entire llvm project yesterday, so that took a pretty long compiling time before I could start testing again.

clang-tidy/modernize/PassByValueCheck.cpp

   if (!paramReferredExactlyOnce(Ctor, ParamDecl))
     return;
 
+  // If the parameter is trivial to copy, don't move it. Moving a trivivally
+  // copyable type will cause a problem with modernize-pass-by-value
+  if (ParamDecl->getType().isTriviallyCopyableType(*Result.Context)) 
+    return;
+
   auto Diag = diag(ParamDecl->getLocStart(), "pass by value and use std::move");
 
   // Iterate over all declarations of the constructor.

test/clang-tidy/modernize-pass-by-value.cpp

   Movable M;
 };
 
+// Test that types that are trivially copyable will not use std::move. This will
+// cause problems with misc-move-const-arg, as it will revert it.
+struct T {
+  std::array<int, 10> a_;
+  T(std::array<int, 10> a) : a_(a) {}
+};