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

[Mads Ravn via cfe-commits]

madsravn added a comment.

@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.


http://reviews.llvm.org/D20365



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

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.


http://reviews.llvm.org/D20365



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

In http://reviews.llvm.org/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" ;)


http://reviews.llvm.org/D20365



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mads Ravn via cfe-commits]

madsravn closed this revision.
madsravn added a comment.

Code committed.


http://reviews.llvm.org/D20365



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mads Ravn via cfe-commits]

madsravn added a comment.

@Prazek thanks. I will look into it :)


http://reviews.llvm.org/D20365



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Piotr Padlewski via cfe-commits]

Prazek added a subscriber: Prazek.
Prazek added a comment.

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.


http://reviews.llvm.org/D20365



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mads Ravn via cfe-commits]

madsravn added a comment.

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


http://reviews.llvm.org/D20365



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mads Ravn via cfe-commits]

madsravn created this revision.
madsravn added reviewers: alexfh, vsk, djasper, klimek.
madsravn added a subscriber: cfe-commits.

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.

http://reviews.llvm.org/D20365

Files:
  clang-tidy/modernize/PassByValueCheck.cpp
  test/clang-tidy/modernize-pass-by-value.cpp

Index: test/clang-tidy/modernize-pass-by-value.cpp
===
--- test/clang-tidy/modernize-pass-by-value.cpp
+++ test/clang-tidy/modernize-pass-by-value.cpp
@@ -194,3 +194,9 @@
   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 a_;
+  T(std::array a) : a_(a) {}
+};
Index: clang-tidy/modernize/PassByValueCheck.cpp
===
--- clang-tidy/modernize/PassByValueCheck.cpp
+++ clang-tidy/modernize/PassByValueCheck.cpp
@@ -181,6 +181,11 @@
   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.


Index: test/clang-tidy/modernize-pass-by-value.cpp
===
--- test/clang-tidy/modernize-pass-by-value.cpp
+++ test/clang-tidy/modernize-pass-by-value.cpp
@@ -194,3 +194,9 @@
   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 a_;
+  T(std::array a) : a_(a) {}
+};
Index: clang-tidy/modernize/PassByValueCheck.cpp
===
--- clang-tidy/modernize/PassByValueCheck.cpp
+++ clang-tidy/modernize/PassByValueCheck.cpp
@@ -181,6 +181,11 @@
   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.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mads Ravn via cfe-commits]

Hi,

I hereby attach a patch to fix bug no. 27731:
https://llvm.org/bugs/show_bug.cgi?id=27731

Best regards,
Mads Ravn
Index: clang-tidy/modernize/PassByValueCheck.cpp
===
--- clang-tidy/modernize/PassByValueCheck.cpp	(revision 269603)
+++ clang-tidy/modernize/PassByValueCheck.cpp	(working copy)
@@ -181,6 +181,11 @@
   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.
Index: test/clang-tidy/modernize-pass-by-value.cpp
===
--- test/clang-tidy/modernize-pass-by-value.cpp	(revision 269603)
+++ test/clang-tidy/modernize-pass-by-value.cpp	(working copy)
@@ -194,3 +194,9 @@
   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 a_;
+  T(std::array a) : a_(a) {}
+};
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits