fix for clang-tidy modernize-pass-by-value on copy constructor

[Kessoufi]

Summary

disabling the modernize-pass-by-value check on copy constructor as :

• it outputs bad cpp code when combined with modernize-use-default
• with only modernize-pass-by-value enabled it triggers a build error

See Bug comments for more details/ example code :
https://llvm.org/bugs/show_bug.cgi?id=27388

[alexfh]

clang-tidy/modernize/PassByValueCheck.cpp

166

You should use unless(isCopyConstructor) instead of creating a separate matcher.

[Kessoufi]

removed the redundant matcher

[Kessoufi]

Done

[alexfh]

Now the problem is that the check changed behavior, but the tests are not updated. Do the tests still pass? If yes, we need to add tests for this behavior. If no, please fix the tests.

[Kessoufi]

• my previous fix was applied on the wrong location and accidentally worked.

now applied on the right location

• previous tests still pass
• added a specific test to highlight my issue

[alexfh]

This test doesn't fail without your fix.

clang-tidy/modernize/PassByValueCheck.cpp

155

This submatcher is much cheaper, so it should go first to make the whole matcher more efficient.

clang-tidy/modernize/PassByValueCheck.cpp

                             isCopyConstructor(), unless(isDeleted()),
                             hasDeclContext(
                                 cxxRecordDecl(isMoveConstructible())))))))
-                    .bind("Initializer")))
+                    .bind("Initializer")),
+             unless(isCopyConstructor()))
             .bind("Ctor"),
         this);
   }

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

   Movable M;
 };
 
+// Test exclusion of copy constructors
+// allowing modernize-pass-by-value on copy constructors causes:
+//  - compiler error: copy constructor must pass its first argument by
+//    reference
+//  - conflict when applying replacement for both modernize-pass-by-value and
+//    modernize-use-default
+struct T {
+    T(const T&) {}
+};