[clang-tools-extra] fc1b24d - [clang-tidy]performance-unnecessary-copy-initialization: fix false negative

[Clement Courbet via cfe-commits]

Author: Clement Courbet
Date: 2021-10-29T08:41:03+02:00
New Revision: fc1b24d7360f59c8b89f1b6290fe849a6207dc44

URL: https://github.com/llvm/llvm-project/commit/fc1b24d7360f59c8b89f1b6290fe849a6207dc44
DIFF: https://github.com/llvm/llvm-project/commit/fc1b24d7360f59c8b89f1b6290fe849a6207dc44.diff

LOG: [clang-tidy]performance-unnecessary-copy-initialization: fix false negative

We're missing all cases where the return value is a type alias.

Unfortunately, this includes things we care about, such as
`std::vector<T>::operator[]` (return value is `const_reference`,
not `const T&`).

Match the canonical type instead.

Differential Revision: https://reviews.llvm.org/D112722

Added: 
    

Modified: 
    clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
    clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 6f9495fd1e66c..2cdd7827ee42a 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -84,7 +84,8 @@ AST_MATCHER_FUNCTION_P(StatementMatcher, isConstRefReturningMethodCall,
   // returned either points to a global static variable or to a member of the
   // called object.
   return cxxMemberCallExpr(
-      callee(cxxMethodDecl(returns(matchers::isReferenceToConst()))
+      callee(cxxMethodDecl(
+                 returns(hasCanonicalType(matchers::isReferenceToConst())))
                  .bind(MethodDeclId)),
       on(declRefExpr(to(
           varDecl(
@@ -97,7 +98,8 @@ AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningFunctionCall) {
   // Only allow initialization of a const reference from a free function if it
   // has no arguments. Otherwise it could return an alias to one of its
   // arguments and the arguments need to be checked for const use as well.
-  return callExpr(callee(functionDecl(returns(matchers::isReferenceToConst()))
+  return callExpr(callee(functionDecl(returns(hasCanonicalType(
+                                          matchers::isReferenceToConst())))
                              .bind(FunctionDeclId)),
                   argumentCountIs(0), unless(callee(cxxMethodDecl())))
       .bind(InitFunctionCallId);

diff  --git a/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
index 68a010c4d953c..4c469c966860b 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -12,6 +12,8 @@ struct ExpensiveToCopyType {
   ExpensiveToCopyType();
   virtual ~ExpensiveToCopyType();
   const ExpensiveToCopyType &reference() const;
+  using ConstRef = const ExpensiveToCopyType &;
+  ConstRef referenceWithAlias() const;
   const ExpensiveToCopyType *pointer() const;
   Iterator<ExpensiveToCopyType> begin() const;
   Iterator<ExpensiveToCopyType> end() const;
@@ -206,6 +208,15 @@ void positiveNonConstVarInCodeBlock(const ExpensiveToCopyType &Obj) {
   }
 }
 
+void positiveNonConstVarInCodeBlockWithAlias(const ExpensiveToCopyType &Obj) {
+  {
+    const ExpensiveToCopyType Assigned = Obj.referenceWithAlias();
+    // CHECK-MESSAGES: [[@LINE-1]]:31: warning: the const qualified variable
+    // CHECK-FIXES: const ExpensiveToCopyType& Assigned = Obj.referenceWithAlias();
+    useAsConstReference(Assigned);
+  }
+}
+
 void negativeNonConstVarWithNonConstUse(const ExpensiveToCopyType &Obj) {
   {
     auto NonConstInvoked = Obj.reference();