[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-initialization: Enhance the check for the scenario with MemberExpr initialization. (PR #151936)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

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

Author: henrywong (movie-travel-code)

<details>
<summary>Changes</summary>

For now, performance-unnecessary-copy-initialization skip the initialization with the `MemberExpr`, see https://github.com/llvm/llvm-project/issues/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.
```cpp
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
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;
+}

``````````

</details>


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