[PATCH] D90042: [clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type.

[Felix Berger via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGace9653c11c6: [clang-tidy] 
performance-unnecessary-copy-initialization: Check for const… (authored by flx).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90042/new/

https://reviews.llvm.org/D90042

Files:
  clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -411,12 +411,12 @@
 
 template 
 class function;
-template 
-class function {
+template 
+class function {
 public:
   function();
-  function(const function );
-  R operator()(Args &&...args) const;
+  function(const function );
+  R operator()(ArgTypes... Args) const;
 };
 
 } // namespace __1
@@ -460,3 +460,19 @@
 }
 
 } // namespace fake
+
+void positiveInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'Copy' is 
copy-constructed from a const reference
+  // CHECK-FIXES: const auto& Copy = Orig.reference();
+  Update(Copy);
+}
+
+void negativeInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  Update(Copy);
+}
Index: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -59,9 +59,13 @@
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   auto ConstReferenceOrValue =
   qualType(anyOf(referenceType(pointee(qualType(isConstQualified(,
- unless(anyOf(referenceType(), pointerType();
+ unless(anyOf(referenceType(), pointerType(),
+  substTemplateTypeParmType();
+  auto ConstReferenceOrValueOrReplaced = qualType(anyOf(
+  ConstReferenceOrValue,
+  substTemplateTypeParmType(hasReplacementType(ConstReferenceOrValue;
   auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
-  DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValue)));
+  DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValueOrReplaced)));
   Matches = match(findAll(callExpr(UsedAsConstRefOrValueArg)), Stmt, Context);
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   Matches =


Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -411,12 +411,12 @@
 
 template 
 class function;
-template 
-class function {
+template 
+class function {
 public:
   function();
-  function(const function );
-  R operator()(Args &&...args) const;
+  function(const function );
+  R operator()(ArgTypes... Args) const;
 };
 
 } // namespace __1
@@ -460,3 +460,19 @@
 }
 
 } // namespace fake
+
+void positiveInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'Copy' is copy-constructed from a const reference
+  // CHECK-FIXES: const auto& Copy = Orig.reference();
+  Update(Copy);
+}
+
+void negativeInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  Update(Copy);
+}
Index: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -59,9 +59,13 @@
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   auto ConstReferenceOrValue =
   qualType(anyOf(referenceType(pointee(qualType(isConstQualified(,
- unless(anyOf(referenceType(), pointerType();
+ unless(anyOf(referenceType(), pointerType(),
+  substTemplateTypeParmType();
+  auto ConstReferenceOrValueOrReplaced = qualType(anyOf(
+  ConstReferenceOrValue,
+  substTemplateTypeParmType(hasReplacementType(ConstReferenceOrValue;
   auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
-  DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValue)));
+  DeclRefToVar, 

[PATCH] D90042: [clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type.

[Felix Berger via Phabricator via cfe-commits]

flx added a comment.

In D90042#2397885 , @aaron.ballman 
wrote:

> LGTM, thank you for the fix!

Thanks for helping track down the difference in our definition of std::function 
:)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90042/new/

https://reviews.llvm.org/D90042

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

[PATCH] D90042: [clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM, thank you for the fix!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90042/new/

https://reviews.llvm.org/D90042

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

[PATCH] D90042: [clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type.

[Felix Berger via Phabricator via cfe-commits]

flx updated this revision to Diff 304834.
flx added a comment.

Updated change description.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90042/new/

https://reviews.llvm.org/D90042

Files:
  clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -411,12 +411,12 @@
 
 template 
 class function;
-template 
-class function {
+template 
+class function {
 public:
   function();
-  function(const function );
-  R operator()(Args &&...args) const;
+  function(const function );
+  R operator()(ArgTypes... Args) const;
 };
 
 } // namespace __1
@@ -460,3 +460,19 @@
 }
 
 } // namespace fake
+
+void positiveInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'Copy' is 
copy-constructed from a const reference
+  // CHECK-FIXES: const auto& Copy = Orig.reference();
+  Update(Copy);
+}
+
+void negativeInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  Update(Copy);
+}
Index: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -59,9 +59,13 @@
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   auto ConstReferenceOrValue =
   qualType(anyOf(referenceType(pointee(qualType(isConstQualified(,
- unless(anyOf(referenceType(), pointerType();
+ unless(anyOf(referenceType(), pointerType(),
+  substTemplateTypeParmType();
+  auto ConstReferenceOrValueOrReplaced = qualType(anyOf(
+  ConstReferenceOrValue,
+  substTemplateTypeParmType(hasReplacementType(ConstReferenceOrValue;
   auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
-  DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValue)));
+  DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValueOrReplaced)));
   Matches = match(findAll(callExpr(UsedAsConstRefOrValueArg)), Stmt, Context);
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   Matches =


Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -411,12 +411,12 @@
 
 template 
 class function;
-template 
-class function {
+template 
+class function {
 public:
   function();
-  function(const function );
-  R operator()(Args &&...args) const;
+  function(const function );
+  R operator()(ArgTypes... Args) const;
 };
 
 } // namespace __1
@@ -460,3 +460,19 @@
 }
 
 } // namespace fake
+
+void positiveInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'Copy' is copy-constructed from a const reference
+  // CHECK-FIXES: const auto& Copy = Orig.reference();
+  Update(Copy);
+}
+
+void negativeInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  Update(Copy);
+}
Index: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -59,9 +59,13 @@
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   auto ConstReferenceOrValue =
   qualType(anyOf(referenceType(pointee(qualType(isConstQualified(,
- unless(anyOf(referenceType(), pointerType();
+ unless(anyOf(referenceType(), pointerType(),
+  substTemplateTypeParmType();
+  auto ConstReferenceOrValueOrReplaced = qualType(anyOf(
+  ConstReferenceOrValue,
+  substTemplateTypeParmType(hasReplacementType(ConstReferenceOrValue;
   auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
-  DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValue)));
+  DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValueOrReplaced)));
   Matches = match(findAll(callExpr(UsedAsConstRefOrValueArg)), Stmt, 

[PATCH] D90042: [clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type.

[Felix Berger via Phabricator via cfe-commits]

flx added a comment.

In D90042#2391246 , @aaron.ballman 
wrote:

> In D90042#2390203 , @flx wrote:
>
>> Thanks for the suggestion, I had never hear of creduce!
>
> Glad to have introduced you to it -- it's a great tool!
>
>> After a bit of trial an error I seem to have found a more minimal example:
>>
>>   namespace std {
>>   
>>   template  class function;
>>   
>>   template  class function { 
>>   
>>   public:
>>   
>> void operator()(b...);   
>>   
>>   }; 
>>   
>>   } // namespace std 
>>   
>>   struct c { 
>>   
>> c(); 
>>   
>> c(const c &);
>>   
>>   }; 
>>   
>>   std::function f;
>>   
>>   void d() { 
>>   
>> c Orig;  
>>   
>> c Copy = Orig;   
>>   
>> f(Copy); 
>>   
>>   }  
>>
>>
>> To be frank I can't spot a meaningful difference to the std::function copy 
>> we already have.
>
> Aha, I may have spotted it. The call operators have subtly different 
> signatures and the signature we have in our test file is wrong. Note the `&&` 
> in our test file compared to what the standard defines: 
> http://eel.is/c++draft/func.wrap.func#inv which is what's causing the 
> difference here: https://godbolt.org/z/hxfM7P

Ah! That is subtle and surprising, but makes sense. I confirmed that the new 
test cases fail now before the fix is put in place. Thanks for your help!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90042/new/

https://reviews.llvm.org/D90042

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

[PATCH] D90042: [clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type.

[Felix Berger via Phabricator via cfe-commits]

flx updated this revision to Diff 304833.
flx added a comment.

Remove unnecessary test code that is not needed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90042/new/

https://reviews.llvm.org/D90042

Files:
  clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -411,12 +411,12 @@
 
 template 
 class function;
-template 
-class function {
+template 
+class function {
 public:
   function();
-  function(const function );
-  R operator()(Args &&...args) const;
+  function(const function );
+  R operator()(ArgTypes... Args) const;
 };
 
 } // namespace __1
@@ -460,3 +460,19 @@
 }
 
 } // namespace fake
+
+void positiveInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'Copy' is 
copy-constructed from a const reference
+  // CHECK-FIXES: const auto& Copy = Orig.reference();
+  Update(Copy);
+}
+
+void negativeInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  Update(Copy);
+}
Index: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -59,9 +59,13 @@
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   auto ConstReferenceOrValue =
   qualType(anyOf(referenceType(pointee(qualType(isConstQualified(,
- unless(anyOf(referenceType(), pointerType();
+ unless(anyOf(referenceType(), pointerType(),
+  substTemplateTypeParmType();
+  auto ConstReferenceOrValueOrReplaced = qualType(anyOf(
+  ConstReferenceOrValue,
+  substTemplateTypeParmType(hasReplacementType(ConstReferenceOrValue;
   auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
-  DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValue)));
+  DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValueOrReplaced)));
   Matches = match(findAll(callExpr(UsedAsConstRefOrValueArg)), Stmt, Context);
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   Matches =


Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -411,12 +411,12 @@
 
 template 
 class function;
-template 
-class function {
+template 
+class function {
 public:
   function();
-  function(const function );
-  R operator()(Args &&...args) const;
+  function(const function );
+  R operator()(ArgTypes... Args) const;
 };
 
 } // namespace __1
@@ -460,3 +460,19 @@
 }
 
 } // namespace fake
+
+void positiveInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'Copy' is copy-constructed from a const reference
+  // CHECK-FIXES: const auto& Copy = Orig.reference();
+  Update(Copy);
+}
+
+void negativeInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  Update(Copy);
+}
Index: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -59,9 +59,13 @@
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   auto ConstReferenceOrValue =
   qualType(anyOf(referenceType(pointee(qualType(isConstQualified(,
- unless(anyOf(referenceType(), pointerType();
+ unless(anyOf(referenceType(), pointerType(),
+  substTemplateTypeParmType();
+  auto ConstReferenceOrValueOrReplaced = qualType(anyOf(
+  ConstReferenceOrValue,
+  substTemplateTypeParmType(hasReplacementType(ConstReferenceOrValue;
   auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
-  DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValue)));
+  DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValueOrReplaced)));
   Matches = 

[PATCH] D90042: [clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type.

[Felix Berger via Phabricator via cfe-commits]

flx updated this revision to Diff 304831.
flx added a comment.

Fixed definition of fake std::function which now makes the bug fixed by this 
change reproducible.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90042/new/

https://reviews.llvm.org/D90042

Files:
  clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -411,14 +411,37 @@
 
 template 
 class function;
-template 
-class function {
+template 
+class function {
 public:
   function();
-  function(const function );
-  R operator()(Args &&...args) const;
+  function(const function );
+  R operator()(ArgTypes... Args) const;
+};
+
+template 
+struct remove_reference;
+
+template 
+struct remove_reference {
+  typedef _Tp type;
+};
+
+template 
+struct remove_reference<_Tp &> {
+  typedef _Tp type;
+};
+
+template 
+struct remove_reference<_Tp &&> {
+  typedef _Tp type;
 };
 
+template 
+constexpr _Tp &(typename remove_reference<_Tp>::type &__t) noexcept {
+  return static_cast<_Tp &&>(__t);
+}
+
 } // namespace __1
 } // namespace std
 
@@ -460,3 +483,19 @@
 }
 
 } // namespace fake
+
+void positiveInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'Copy' is 
copy-constructed from a const reference
+  // CHECK-FIXES: const auto& Copy = Orig.reference();
+  Update(Copy);
+}
+
+void negativeInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  Update(Copy);
+}
Index: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -59,9 +59,13 @@
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   auto ConstReferenceOrValue =
   qualType(anyOf(referenceType(pointee(qualType(isConstQualified(,
- unless(anyOf(referenceType(), pointerType();
+ unless(anyOf(referenceType(), pointerType(),
+  substTemplateTypeParmType();
+  auto ConstReferenceOrValueOrReplaced = qualType(anyOf(
+  ConstReferenceOrValue,
+  substTemplateTypeParmType(hasReplacementType(ConstReferenceOrValue;
   auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
-  DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValue)));
+  DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValueOrReplaced)));
   Matches = match(findAll(callExpr(UsedAsConstRefOrValueArg)), Stmt, Context);
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   Matches =


Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -411,14 +411,37 @@
 
 template 
 class function;
-template 
-class function {
+template 
+class function {
 public:
   function();
-  function(const function );
-  R operator()(Args &&...args) const;
+  function(const function );
+  R operator()(ArgTypes... Args) const;
+};
+
+template 
+struct remove_reference;
+
+template 
+struct remove_reference {
+  typedef _Tp type;
+};
+
+template 
+struct remove_reference<_Tp &> {
+  typedef _Tp type;
+};
+
+template 
+struct remove_reference<_Tp &&> {
+  typedef _Tp type;
 };
 
+template 
+constexpr _Tp &(typename remove_reference<_Tp>::type &__t) noexcept {
+  return static_cast<_Tp &&>(__t);
+}
+
 } // namespace __1
 } // namespace std
 
@@ -460,3 +483,19 @@
 }
 
 } // namespace fake
+
+void positiveInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'Copy' is copy-constructed from a const reference
+  // CHECK-FIXES: const auto& Copy = Orig.reference();
+  Update(Copy);
+}
+
+void negativeInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  Update(Copy);
+}
Index: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ 

[PATCH] D90042: [clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D90042#2390203 , @flx wrote:

> Thanks for the suggestion, I had never hear of creduce!

Glad to have introduced you to it -- it's a great tool!

> After a bit of trial an error I seem to have found a more minimal example:
>
>   namespace std { 
>  
>   template  class function; 
>  
>   template  class function {  
>  
>   public: 
>  
> void operator()(b...);
>  
>   };  
>  
>   } // namespace std  
>  
>   struct c {  
>  
> c();  
>  
> c(const c &); 
>  
>   };  
>  
>   std::function f; 
>  
>   void d() {  
>  
> c Orig;   
>  
> c Copy = Orig;
>  
> f(Copy);  
>  
>   }  
>
>
> To be frank I can't spot a meaningful difference to the std::function copy we 
> already have.

Aha, I may have spotted it. The call operators have subtly different signatures 
and the signature we have in our test file is wrong. Note the `&&` in our test 
file compared to what the standard defines: 
http://eel.is/c++draft/func.wrap.func#inv which is what's causing the 
difference here: https://godbolt.org/z/hxfM7P


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90042/new/

https://reviews.llvm.org/D90042

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

[PATCH] D90042: [clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type.

[Felix Berger via Phabricator via cfe-commits]

flx added a comment.

In D90042#2368219 , @aaron.ballman 
wrote:

> In D90042#2360042 , @flx wrote:
>
>> In D90042#2357078 , @aaron.ballman 
>> wrote:
>>
>>> In D90042#2356265 , @flx wrote:
>>>
 In D90042#2356180 , 
 @aaron.ballman wrote:

> In D90042#2350035 , @flx wrote:
>
>> I should note that I was only able to reproduce the false positive with 
>> the actual implementation std::function and not our fake version here.
>
> Any reason not to lift enough of the actual definition to be able to 
> reproduce the issue in your test cases? Does the change in definitions 
> break other tests?

 I poured over the actual definition and couldn't find any difference wrt 
 the call operator that would explain it. I would also think that:

   template 
   void foo(T&& t) {
 std::forward(t).modify();
   }

 would be a simpler case that should trigger replacement, but it doesn't. 
 Do you have any idea what I could be missing?
>>>
>>> Perhaps silly question, but are you instantiating `foo()`?
>>
>> I think I added a full implementation of foo now, reverted the change, but 
>> am still not getting the negative case to fail. Can you spot an issue with 
>> the code?
>
> I can't, but to be honest, I'm not certain I understand how that false 
> positive could happen in the first place. That's why I was hoping to see the 
> original case -- one thing you could try is with the original code, pass `-E` 
> to preprocess to a file, and then try reducing the test case from that output 
> (either by hand or by using a tool like creduce), or did you already give 
> that a shot?

Thanks for the suggestion, I had never hear of creduce! After a bit of trial an 
error I seem to have found a more minimal example:

  namespace std {   
   
  template  class function;   
   
  template  class function {
   
  public:   
   
void operator()(b...);  
   
  };
   
  } // namespace std
   
  struct c {
   
c();
   
c(const c &);   
   
  };
   
  std::function f;   
   
  void d() {
   
c Orig; 
   
c Copy = Orig;  
   
f(Copy);
   
  }  

To be frank I can't spot a meaningful difference to the std::function copy we 
already have.

Here's also the test script I used for posterity:

  #!/bin/bash   
   

   
  trap 'exit 1' ERR 
   
  out=$(tempfile)   
   
  clang-tidy  --checks=-*,performance-unnecessary-copy-initialization 
--extra-arg=-std=c++17 full.cc > $out 
  grep "warning: local copy.*Copy" $out 
  

[PATCH] D90042: [clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D90042#2360042 , @flx wrote:

> In D90042#2357078 , @aaron.ballman 
> wrote:
>
>> In D90042#2356265 , @flx wrote:
>>
>>> In D90042#2356180 , @aaron.ballman 
>>> wrote:
>>>
 In D90042#2350035 , @flx wrote:

> I should note that I was only able to reproduce the false positive with 
> the actual implementation std::function and not our fake version here.

 Any reason not to lift enough of the actual definition to be able to 
 reproduce the issue in your test cases? Does the change in definitions 
 break other tests?
>>>
>>> I poured over the actual definition and couldn't find any difference wrt 
>>> the call operator that would explain it. I would also think that:
>>>
>>>   template 
>>>   void foo(T&& t) {
>>> std::forward(t).modify();
>>>   }
>>>
>>> would be a simpler case that should trigger replacement, but it doesn't. Do 
>>> you have any idea what I could be missing?
>>
>> Perhaps silly question, but are you instantiating `foo()`?
>
> I think I added a full implementation of foo now, reverted the change, but am 
> still not getting the negative case to fail. Can you spot an issue with the 
> code?

I can't, but to be honest, I'm not certain I understand how that false positive 
could happen in the first place. That's why I was hoping to see the original 
case -- one thing you could try is with the original code, pass `-E` to 
preprocess to a file, and then try reducing the test case from that output 
(either by hand or by using a tool like creduce), or did you already give that 
a shot?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90042/new/

https://reviews.llvm.org/D90042

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

[PATCH] D90042: [clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type.

[Felix Berger via Phabricator via cfe-commits]

flx added a comment.

In D90042#2357078 , @aaron.ballman 
wrote:

> In D90042#2356265 , @flx wrote:
>
>> In D90042#2356180 , @aaron.ballman 
>> wrote:
>>
>>> In D90042#2350035 , @flx wrote:
>>>
 I should note that I was only able to reproduce the false positive with 
 the actual implementation std::function and not our fake version here.
>>>
>>> Any reason not to lift enough of the actual definition to be able to 
>>> reproduce the issue in your test cases? Does the change in definitions 
>>> break other tests?
>>
>> I poured over the actual definition and couldn't find any difference wrt the 
>> call operator that would explain it. I would also think that:
>>
>>   template 
>>   void foo(T&& t) {
>> std::forward(t).modify();
>>   }
>>
>> would be a simpler case that should trigger replacement, but it doesn't. Do 
>> you have any idea what I could be missing?
>
> Perhaps silly question, but are you instantiating `foo()`?

I think I added a full implementation of foo now, reverted the change, but am 
still not getting the negative case to fail. Can you spot an issue with the 
code?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90042/new/

https://reviews.llvm.org/D90042

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

[PATCH] D90042: [clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type.

[Felix Berger via Phabricator via cfe-commits]

flx updated this revision to Diff 301389.
flx added a comment.

Comment out replaced parameter.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90042/new/

https://reviews.llvm.org/D90042

Files:
  clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -411,14 +411,37 @@
 
 template 
 class function;
-template 
-class function {
+template 
+class function {
 public:
   function();
-  function(const function );
-  R operator()(Args &&...args) const;
+  function(const function );
+  R operator()(ArgTypes &&...Args) const;
+};
+
+template 
+struct remove_reference;
+
+template 
+struct remove_reference {
+  typedef _Tp type;
+};
+
+template 
+struct remove_reference<_Tp &> {
+  typedef _Tp type;
+};
+
+template 
+struct remove_reference<_Tp &&> {
+  typedef _Tp type;
 };
 
+template 
+constexpr _Tp &(typename remove_reference<_Tp>::type &__t) noexcept {
+  return static_cast<_Tp &&>(__t);
+}
+
 } // namespace __1
 } // namespace std
 
@@ -460,3 +483,24 @@
 }
 
 } // namespace fake
+
+void positiveInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'Copy' is 
copy-constructed from a const reference
+  // CHECK-FIXES: const auto& Copy = Orig.reference();
+  Update(Copy);
+}
+
+template 
+void foo(Type &) {
+  std::forward(T).nonConstMethod();
+}
+
+void negativeInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  foo(Copy);
+}
Index: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -59,7 +59,11 @@
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   auto ConstReferenceOrValue =
   qualType(anyOf(referenceType(pointee(qualType(isConstQualified(,
- unless(anyOf(referenceType(), pointerType();
+ unless(anyOf(referenceType(), pointerType()/*,
+  
substTemplateTypeParmType()*/;
+  auto ConstReferenceOrValueOrReplaced = qualType(anyOf(
+  ConstReferenceOrValue,
+  substTemplateTypeParmType(hasReplacementType(ConstReferenceOrValue;
   auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
   DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValue)));
   Matches = match(findAll(callExpr(UsedAsConstRefOrValueArg)), Stmt, Context);


Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -411,14 +411,37 @@
 
 template 
 class function;
-template 
-class function {
+template 
+class function {
 public:
   function();
-  function(const function );
-  R operator()(Args &&...args) const;
+  function(const function );
+  R operator()(ArgTypes &&...Args) const;
+};
+
+template 
+struct remove_reference;
+
+template 
+struct remove_reference {
+  typedef _Tp type;
+};
+
+template 
+struct remove_reference<_Tp &> {
+  typedef _Tp type;
+};
+
+template 
+struct remove_reference<_Tp &&> {
+  typedef _Tp type;
 };
 
+template 
+constexpr _Tp &(typename remove_reference<_Tp>::type &__t) noexcept {
+  return static_cast<_Tp &&>(__t);
+}
+
 } // namespace __1
 } // namespace std
 
@@ -460,3 +483,24 @@
 }
 
 } // namespace fake
+
+void positiveInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'Copy' is copy-constructed from a const reference
+  // CHECK-FIXES: const auto& Copy = Orig.reference();
+  Update(Copy);
+}
+
+template 
+void foo(Type &) {
+  std::forward(T).nonConstMethod();
+}
+
+void negativeInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  foo(Copy);
+}
Index: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp

[PATCH] D90042: [clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type.

[Felix Berger via Phabricator via cfe-commits]

flx updated this revision to Diff 301388.
flx added a comment.

Add instantiated template method.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90042/new/

https://reviews.llvm.org/D90042

Files:
  clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -411,14 +411,37 @@
 
 template 
 class function;
-template 
-class function {
+template 
+class function {
 public:
   function();
-  function(const function );
-  R operator()(Args &&...args) const;
+  function(const function );
+  R operator()(ArgTypes &&...Args) const;
+};
+
+template 
+struct remove_reference;
+
+template 
+struct remove_reference {
+  typedef _Tp type;
+};
+
+template 
+struct remove_reference<_Tp &> {
+  typedef _Tp type;
+};
+
+template 
+struct remove_reference<_Tp &&> {
+  typedef _Tp type;
 };
 
+template 
+constexpr _Tp &(typename remove_reference<_Tp>::type &__t) noexcept {
+  return static_cast<_Tp &&>(__t);
+}
+
 } // namespace __1
 } // namespace std
 
@@ -460,3 +483,24 @@
 }
 
 } // namespace fake
+
+void positiveInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'Copy' is 
copy-constructed from a const reference
+  // CHECK-FIXES: const auto& Copy = Orig.reference();
+  Update(Copy);
+}
+
+template 
+void foo(Type &) {
+  std::forward(T).nonConstMethod();
+}
+
+void negativeInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  foo(Copy);
+}
Index: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -59,7 +59,11 @@
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   auto ConstReferenceOrValue =
   qualType(anyOf(referenceType(pointee(qualType(isConstQualified(,
- unless(anyOf(referenceType(), pointerType();
+ unless(anyOf(referenceType(), pointerType(),
+  substTemplateTypeParmType();
+  auto ConstReferenceOrValueOrReplaced = qualType(anyOf(
+  ConstReferenceOrValue,
+  substTemplateTypeParmType(hasReplacementType(ConstReferenceOrValue;
   auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
   DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValue)));
   Matches = match(findAll(callExpr(UsedAsConstRefOrValueArg)), Stmt, Context);


Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -411,14 +411,37 @@
 
 template 
 class function;
-template 
-class function {
+template 
+class function {
 public:
   function();
-  function(const function );
-  R operator()(Args &&...args) const;
+  function(const function );
+  R operator()(ArgTypes &&...Args) const;
+};
+
+template 
+struct remove_reference;
+
+template 
+struct remove_reference {
+  typedef _Tp type;
+};
+
+template 
+struct remove_reference<_Tp &> {
+  typedef _Tp type;
+};
+
+template 
+struct remove_reference<_Tp &&> {
+  typedef _Tp type;
 };
 
+template 
+constexpr _Tp &(typename remove_reference<_Tp>::type &__t) noexcept {
+  return static_cast<_Tp &&>(__t);
+}
+
 } // namespace __1
 } // namespace std
 
@@ -460,3 +483,24 @@
 }
 
 } // namespace fake
+
+void positiveInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'Copy' is copy-constructed from a const reference
+  // CHECK-FIXES: const auto& Copy = Orig.reference();
+  Update(Copy);
+}
+
+template 
+void foo(Type &) {
+  std::forward(T).nonConstMethod();
+}
+
+void negativeInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  foo(Copy);
+}
Index: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -59,7 +59,11 @@
   

[PATCH] D90042: [clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D90042#2356265 , @flx wrote:

> In D90042#2356180 , @aaron.ballman 
> wrote:
>
>> In D90042#2350035 , @flx wrote:
>>
>>> I should note that I was only able to reproduce the false positive with the 
>>> actual implementation std::function and not our fake version here.
>>
>> Any reason not to lift enough of the actual definition to be able to 
>> reproduce the issue in your test cases? Does the change in definitions break 
>> other tests?
>
> I poured over the actual definition and couldn't find any difference wrt the 
> call operator that would explain it. I would also think that:
>
>   template 
>   void foo(T&& t) {
> std::forward(t).modify();
>   }
>
> would be a simpler case that should trigger replacement, but it doesn't. Do 
> you have any idea what I could be missing?

Perhaps silly question, but are you instantiating `foo()`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90042/new/

https://reviews.llvm.org/D90042

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

[PATCH] D90042: [clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type.

[Felix Berger via Phabricator via cfe-commits]

flx added a comment.

In D90042#2356180 , @aaron.ballman 
wrote:

> In D90042#2350035 , @flx wrote:
>
>> I should note that I was only able to reproduce the false positive with the 
>> actual implementation std::function and not our fake version here.
>
> Any reason not to lift enough of the actual definition to be able to 
> reproduce the issue in your test cases? Does the change in definitions break 
> other tests?

I poured over the actual definition and couldn't find any difference wrt the 
call operator that would explain it. I would also think that:

  template 
  void foo(T&& t) {
std::forward(t).modify();
  }

would be a simpler case that should trigger replacement, but it doesn't. Do you 
have any idea what I could be missing?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90042/new/

https://reviews.llvm.org/D90042

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

[PATCH] D90042: [clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D90042#2350035 , @flx wrote:

> I should note that I was only able to reproduce the false positive with the 
> actual implementation std::function and not our fake version here.

Any reason not to lift enough of the actual definition to be able to reproduce 
the issue in your test cases? Does the change in definitions break other tests?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90042/new/

https://reviews.llvm.org/D90042

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

[PATCH] D90042: [clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type.

[Felix Berger via Phabricator via cfe-commits]

flx added a comment.

I should note that I was only able to reproduce the false positive with the 
actual implementation std::function and not our fake version here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90042/new/

https://reviews.llvm.org/D90042

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

[PATCH] D90042: [clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type.

[Felix Berger via Phabricator via cfe-commits]

flx updated this revision to Diff 300276.
flx added a comment.

Revert -std=c++17 change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90042/new/

https://reviews.llvm.org/D90042

Files:
  clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -411,12 +411,12 @@
 
 template 
 class function;
-template 
-class function {
+template 
+class function {
 public:
   function();
-  function(const function );
-  R operator()(Args &&...args) const;
+  function(const function );
+  R operator()(ArgTypes &&...Args) const;
 };
 
 } // namespace __1
@@ -460,3 +460,19 @@
 }
 
 } // namespace fake
+
+void positiveInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'Copy' is 
copy-constructed from a const reference
+  // CHECK-FIXES: const auto& Copy = Orig.reference();
+  Update(Copy);
+}
+
+void negativeInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  Update(Copy);
+}
Index: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -59,9 +59,13 @@
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   auto ConstReferenceOrValue =
   qualType(anyOf(referenceType(pointee(qualType(isConstQualified(,
- unless(anyOf(referenceType(), pointerType();
+ unless(anyOf(referenceType(), pointerType(),
+  substTemplateTypeParmType();
+  auto ConstReferenceOrValueOrReplaced = qualType(anyOf(
+  ConstReferenceOrValue,
+  substTemplateTypeParmType(hasReplacementType(ConstReferenceOrValue;
   auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
-  DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValue)));
+  DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValueOrReplaced)));
   Matches = match(findAll(callExpr(UsedAsConstRefOrValueArg)), Stmt, Context);
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   Matches =


Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -411,12 +411,12 @@
 
 template 
 class function;
-template 
-class function {
+template 
+class function {
 public:
   function();
-  function(const function );
-  R operator()(Args &&...args) const;
+  function(const function );
+  R operator()(ArgTypes &&...Args) const;
 };
 
 } // namespace __1
@@ -460,3 +460,19 @@
 }
 
 } // namespace fake
+
+void positiveInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'Copy' is copy-constructed from a const reference
+  // CHECK-FIXES: const auto& Copy = Orig.reference();
+  Update(Copy);
+}
+
+void negativeInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  Update(Copy);
+}
Index: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -59,9 +59,13 @@
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   auto ConstReferenceOrValue =
   qualType(anyOf(referenceType(pointee(qualType(isConstQualified(,
- unless(anyOf(referenceType(), pointerType();
+ unless(anyOf(referenceType(), pointerType(),
+  substTemplateTypeParmType();
+  auto ConstReferenceOrValueOrReplaced = qualType(anyOf(
+  ConstReferenceOrValue,
+  substTemplateTypeParmType(hasReplacementType(ConstReferenceOrValue;
   auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
-  DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValue)));
+  DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValueOrReplaced)));
   Matches = match(findAll(callExpr(UsedAsConstRefOrValueArg)), Stmt, 

[PATCH] D90042: [clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type.

[Felix Berger via Phabricator via cfe-commits]

flx updated this revision to Diff 300274.
flx added a comment.

Remove code comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90042/new/

https://reviews.llvm.org/D90042

Files:
  clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s performance-unnecessary-copy-initialization %t
+// RUN: %check_clang_tidy -std=c++17 %s 
performance-unnecessary-copy-initialization %t
 
 struct ExpensiveToCopyType {
   ExpensiveToCopyType();
@@ -411,12 +411,12 @@
 
 template 
 class function;
-template 
-class function {
+template 
+class function {
 public:
   function();
-  function(const function );
-  R operator()(Args &&...args) const;
+  function(const function );
+  R operator()(ArgTypes &&...Args) const;
 };
 
 } // namespace __1
@@ -460,3 +460,19 @@
 }
 
 } // namespace fake
+
+void positiveInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'Copy' is 
copy-constructed from a const reference
+  // CHECK-FIXES: const auto& Copy = Orig.reference();
+  Update(Copy);
+}
+
+void negativeInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  Update(Copy);
+}
Index: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -59,9 +59,13 @@
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   auto ConstReferenceOrValue =
   qualType(anyOf(referenceType(pointee(qualType(isConstQualified(,
- unless(anyOf(referenceType(), pointerType();
+ unless(anyOf(referenceType(), pointerType(),
+  substTemplateTypeParmType();
+  auto ConstReferenceOrValueOrReplaced = qualType(anyOf(
+  ConstReferenceOrValue,
+  substTemplateTypeParmType(hasReplacementType(ConstReferenceOrValue;
   auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
-  DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValue)));
+  DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValueOrReplaced)));
   Matches = match(findAll(callExpr(UsedAsConstRefOrValueArg)), Stmt, Context);
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   Matches =


Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s performance-unnecessary-copy-initialization %t
+// RUN: %check_clang_tidy -std=c++17 %s performance-unnecessary-copy-initialization %t
 
 struct ExpensiveToCopyType {
   ExpensiveToCopyType();
@@ -411,12 +411,12 @@
 
 template 
 class function;
-template 
-class function {
+template 
+class function {
 public:
   function();
-  function(const function );
-  R operator()(Args &&...args) const;
+  function(const function );
+  R operator()(ArgTypes &&...Args) const;
 };
 
 } // namespace __1
@@ -460,3 +460,19 @@
 }
 
 } // namespace fake
+
+void positiveInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'Copy' is copy-constructed from a const reference
+  // CHECK-FIXES: const auto& Copy = Orig.reference();
+  Update(Copy);
+}
+
+void negativeInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  Update(Copy);
+}
Index: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -59,9 +59,13 @@
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   auto ConstReferenceOrValue =
   qualType(anyOf(referenceType(pointee(qualType(isConstQualified(,
- unless(anyOf(referenceType(), pointerType();
+ unless(anyOf(referenceType(), pointerType(),
+  

[PATCH] D90042: [clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type.

[Felix Berger via Phabricator via cfe-commits]

flx created this revision.
flx added reviewers: aaron.ballman, gribozavr2.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.
flx requested review of this revision.

This fixes false positive cases where a non-const reference is passed to a
std::function but interpreted as a const reference.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90042

Files:
  clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s performance-unnecessary-copy-initialization %t
+// RUN: %check_clang_tidy -std=c++17 %s 
performance-unnecessary-copy-initialization %t
 
 struct ExpensiveToCopyType {
   ExpensiveToCopyType();
@@ -411,12 +411,12 @@
 
 template 
 class function;
-template 
-class function {
+template 
+class function {
 public:
   function();
-  function(const function );
-  R operator()(Args &&...args) const;
+  function(const function );
+  R operator()(ArgTypes &&...Args) const;
 };
 
 } // namespace __1
@@ -460,3 +460,19 @@
 }
 
 } // namespace fake
+
+void positiveInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'Copy' is 
copy-constructed from a const reference
+  // CHECK-FIXES: const auto& Copy = Orig.reference();
+  Update(Copy);
+}
+
+void negativeInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  Update(Copy);
+}
Index: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -57,11 +57,15 @@
   Stmt, Context);
   SmallPtrSet DeclRefs;
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
-  auto ConstReferenceOrValue =
-  qualType(anyOf(referenceType(pointee(qualType(isConstQualified(,
- unless(anyOf(referenceType(), pointerType();
+  auto ConstReferenceOrValue = qualType(
+  anyOf(referenceType(pointee(qualType(isConstQualified(,
+unless(anyOf(referenceType(),
+ pointerType() /*, substTemplateTypeParmType()*/;
+  auto ConstReferenceOrValueOrReplaced = qualType(anyOf(
+  ConstReferenceOrValue,
+  substTemplateTypeParmType(hasReplacementType(ConstReferenceOrValue;
   auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
-  DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValue)));
+  DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValueOrReplaced)));
   Matches = match(findAll(callExpr(UsedAsConstRefOrValueArg)), Stmt, Context);
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   Matches =


Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s performance-unnecessary-copy-initialization %t
+// RUN: %check_clang_tidy -std=c++17 %s performance-unnecessary-copy-initialization %t
 
 struct ExpensiveToCopyType {
   ExpensiveToCopyType();
@@ -411,12 +411,12 @@
 
 template 
 class function;
-template 
-class function {
+template 
+class function {
 public:
   function();
-  function(const function );
-  R operator()(Args &&...args) const;
+  function(const function );
+  R operator()(ArgTypes &&...Args) const;
 };
 
 } // namespace __1
@@ -460,3 +460,19 @@
 }
 
 } // namespace fake
+
+void positiveInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the variable 'Copy' is copy-constructed from a const reference
+  // CHECK-FIXES: const auto& Copy = Orig.reference();
+  Update(Copy);
+}
+
+void negativeInvokedOnStdFunction(
+std::function Update,
+const ExpensiveToCopyType Orig) {
+  auto Copy = Orig.reference();
+  Update(Copy);
+}
Index: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -57,11 +57,15 @@
   Stmt,