[clang-tidy] performance-unnecessary-copy-initialization: Enhance the check for the scenario with MemberExpr initialization.

[movie-travel-code]

For now, performance-unnecessary-copy-initialization skip the check of the initialization with MemberExpr, see #98005, which lost a large number of optimization opportunities.

This check adds two new checks, as shown in the following code. Given that the analysis of MemberExpr is relatively complex, only the case where the old var object is const-qualified is considered here.

bool CopiedFromConstRefParmVar(const Struct &crs, const Struct cs, Struct &rs, Struct s) {
  const auto m1 = crs.Member; // should warn
  const auto m2 = cs.Member; // should warn
}

Fixes #98005

[llvmbot]

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: henrywong (movie-travel-code)

Changes

For now, performance-unnecessary-copy-initialization skip the initialization with the MemberExpr, see #98005, which lost a large number of optimization opportunities.

This check adds two new checks, as shown in the following code. Given that the analysis of MemberExpr is relatively complex, only the case where the old object is const is considered here.

bool CopiedFromConstRefParmVar(const Struct &crs, const Struct cs, Struct &rs, Struct s) {
  const auto m1 = crs.Member; // should warn
  const auto m2 = cs.Member; // should warn
}

---

Full diff: https://github.com/llvm/llvm-project/pull/151936.diff

3 Files Affected:

• (modified) clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp (+31-1)
• (modified) clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h (+5)
• (modified) clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-initialization.cpp (+25)

diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index b6f19811f5e5c..c8a46f27000e7 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -273,6 +273,15 @@ void UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(LocalVarCopiedFrom(declRefExpr(
                          to(varDecl(hasLocalStorage()).bind(OldVarDeclId)))),
                      this);
+
+  Finder->addMatcher(
+      LocalVarCopiedFrom(memberExpr(
+          hasObjectExpression(expr(ignoringParenImpCasts(declRefExpr(
+              to(varDecl(anyOf(hasType(isConstQualified()),
+                               hasType(references(isConstQualified()))))
+                     .bind(OldVarDeclId)))))),
+          member(fieldDecl().bind("fieldDecl")))),
+      this);
 }
 
 void UnnecessaryCopyInitialization::check(
@@ -294,6 +303,7 @@ void UnnecessaryCopyInitialization::check(
       IssueFix, IsVarUnused, IsVarOnlyUsedAsConst};
   const auto *OldVar = Result.Nodes.getNodeAs<VarDecl>(OldVarDeclId);
   const auto *ObjectArg = Result.Nodes.getNodeAs<VarDecl>(ObjectArgId);
+  const auto *FD = Result.Nodes.getNodeAs<FieldDecl>("fieldDecl");
   const auto *CtorCall = Result.Nodes.getNodeAs<CXXConstructExpr>("ctorCall");
 
   TraversalKindScope RAII(*Result.Context, TK_AsIs);
@@ -318,9 +328,12 @@ void UnnecessaryCopyInitialization::check(
   if (OldVar == nullptr) {
     // `auto NewVar = functionCall();`
     handleCopyFromMethodReturn(Context, ObjectArg);
-  } else {
+  } else if (FD == nullptr){
     // `auto NewVar = OldVar;`
     handleCopyFromLocalVar(Context, *OldVar);
+  } else {
+    // `auto NewVar = OldVar.FD;`
+    handleCopyFromConstLocalVarMember(Context, *OldVar);
   }
 }
 
@@ -345,6 +358,11 @@ void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
   diagnoseCopyFromLocalVar(Ctx, OldVar);
 }
 
+void UnnecessaryCopyInitialization::handleCopyFromConstLocalVarMember(
+    const CheckContext &Ctx, const VarDecl &OldVar) {
+  diagnoseCopyFromConstLocalVarMember(Ctx, OldVar);
+}
+
 void UnnecessaryCopyInitialization::diagnoseCopyFromMethodReturn(
     const CheckContext &Ctx) {
   auto Diagnostic =
@@ -369,6 +387,18 @@ void UnnecessaryCopyInitialization::diagnoseCopyFromLocalVar(
   maybeIssueFixes(Ctx, Diagnostic);
 }
 
+void UnnecessaryCopyInitialization::diagnoseCopyFromConstLocalVarMember(
+    const CheckContext &Ctx, const VarDecl &OldVar) {
+  auto Diagnostic =
+      diag(Ctx.Var.getLocation(),
+           "local copy %1 of the field of the variable %0 is never "
+           "modified%select{"
+           "| and never used}2; consider %select{avoiding the copy|removing "
+           "the statement}2")
+      << &OldVar << &Ctx.Var << Ctx.IsVarUnused;
+  maybeIssueFixes(Ctx, Diagnostic);
+}
+
 void UnnecessaryCopyInitialization::maybeIssueFixes(
     const CheckContext &Ctx, DiagnosticBuilder &Diagnostic) {
   if (Ctx.IssueFix) {
diff --git a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
index 38f756f9b452f..1dade991cfd5f 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
@@ -50,12 +50,17 @@ class UnnecessaryCopyInitialization : public ClangTidyCheck {
   virtual void diagnoseCopyFromMethodReturn(const CheckContext &Ctx);
   virtual void diagnoseCopyFromLocalVar(const CheckContext &Ctx,
                                         const VarDecl &OldVar);
+  virtual void diagnoseCopyFromConstLocalVarMember(const CheckContext &Ctx,
+                                                   const VarDecl &OldVar);
 
 private:
   void handleCopyFromMethodReturn(const CheckContext &Ctx,
                                   const VarDecl *ObjectArg);
   void handleCopyFromLocalVar(const CheckContext &Ctx, const VarDecl &OldVar);
 
+  void handleCopyFromConstLocalVarMember(const CheckContext &Ctx,
+                                         const VarDecl &OldVar);
+
   void maybeIssueFixes(const CheckContext &Ctx, DiagnosticBuilder &Diagnostic);
 
   const std::vector<StringRef> AllowedTypes;
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 c0f1fb9c0f6d2..8f263ddde57b1 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
@@ -939,3 +939,28 @@ template<typename T> bool OperatorWithNoDirectCallee(T t) {
   return a1 == t;
 }
 
+bool CopiedFromConstRefParmVar(const Struct &crs, const Struct cs, Struct &rs, Struct s) {
+  const auto m1 = crs.Member;
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: local copy 'm1' of the field of the variable 'crs' is never modified; consider avoiding the copy
+  // CHECK-FIXES: const auto& m1 = crs.Member;
+  const auto m2 = cs.Member;
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: local copy 'm2' of the field of the variable 'cs' is never modified; consider avoiding the copy
+  // CHECK-FIXES: const auto& m2 = cs.Member;
+  const auto m3 = rs.Member;
+  const auto m4 = s.Member;
+  return m1 == m2 || m3 == m4;
+}
+
+const Struct GlobalS;
+bool CopiedFromConstLocalVar() {
+  const Struct crs;
+  Struct s;
+  const auto m1 = crs.Member;
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: local copy 'm1' of the field of the variable 'crs' is never modified; consider avoiding the copy
+  // CHECK-FIXES: const auto& m1 = crs.Member;
+  const auto m2 = s.Member;
+  const auto m3 = GlobalS.Member;
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: local copy 'm3' of the field of the variable 'GlobalS' is never modified; consider avoiding the copy
+  // CHECK-FIXES: const auto& m3 = GlobalS.Member;
+  return m1 == m2 || m2 == m3;
+}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[EugeneZelenko]

I think changes should be mentioned in Release Notes.

[movie-travel-code]

Hi EugeneZelenko, appreciate your reminding me of that. I have added it.