combining modernize-pass-by-value and modernize-use-default generates bad cpp code

[Quuxplusone]

Bugzilla Link	PR27388
Status	NEW
Importance	P normal
Reported by	kamal essoufi (kessoufi@gmail.com)
Reported on	2016-04-16 20:14:30 -0700
Last modified on	2016-04-16 22:13:51 -0700
Version	unspecified
Hardware	PC Linux
CC	alexfh@google.com, djasper@google.com, kessoufi@gmail.com, klimek@google.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

version > 3.8 (latest sync : git-svn-id: https://llvm.org/svn/llvm-
project/clang-tools-extra/trunk@266265 91177308-0d34-0410-b5e6-96231b3b80d8)

On This code :

// tidy-bug.cpp
class Base {};

class Derived : public Base {
public:
  Derived(const Derived &src);
};
Derived::Derived(const Derived &src) : Base(src) {}

Executing :

clang-tidy -checks=modernize-* tidy-bug.cpp -fix

Reports :

2 warnings generated.
/mypath/tidy-bug.cpp:7:1: warning: use '= default' to define a trivial copy
constructor [modernize-use-default]
Derived::Derived(const Derived &src) : Base(src) {}
^
                                     = default;
/mypath/tidy-bug.cpp:7:38: note: FIX-IT applied suggested code changes
Derived::Derived(const Derived &src) : Base(src) {}
                                     ^
/mypath/tidy-bug.cpp:7:18: warning: pass by value and use std::move [modernize-
pass-by-value]
Derived::Derived(const Derived &src) : Base(src) {}
                 ^
/mypath/tidy-bug.cpp:1:1: note: FIX-IT applied suggested code changes
class Base {};
^
/mypath/tidy-bug.cpp:5:11: note: FIX-IT applied suggested code changes
  Derived(const Derived &src);
          ^
/mypath/tidy-bug.cpp:7:18: note: FIX-IT applied suggested code changes
Derived::Derived(const Derived &src) : Base(src) {}
                 ^
/mypath/tidy-bug.cpp:7:45: note: FIX-IT applied suggested code changes
Derived::Derived(const Derived &src) : Base(src) {}
                                            ^
/mypath/tidy-bug.cpp:7:48: note: FIX-IT applied suggested code changes
Derived::Derived(const Derived &src) : Base(src) {}
                                               ^
clang-tidy applied 6 of 6 suggested fixes.

And generates bad cpp code :

#include <utility>

class Base {};

class Derived : public Base {
public:
  Derived(Derived src);
};
Derived::Derived(Derived src) = dstd::move(efa)ult;

[Quuxplusone]

I found a workaroud (/fix(?)) :

considering that copy constructor is not a good candidate for modernize-pass-by-
value : the *expected* generated code :

Derived(Derived src) : Base(std::move(src)) {}

triggers clang error : copy constructor must pass its first argument by
reference

In PassByValueCheck.cpp, I replaced :

isCopyConstructor(), unless(isDeleted()),

with this matcher :

AST_MATCHER(CXXConstructorDecl, isNotCopyConstructor) {
  return !Node.isCopyConstructor();
}

and only modernize-use-default was applied (correctly) which sounds logical to
me

not sure if this is the right fix though

Regards