[clang-tidy] performance-* checks: Also allow allow member expressions to be used...

Until now when determining all the const uses of a VarDecl we only considered
how the variable itself was used. This change extends checking for const usages
of the type's members as well.

This increases the number of true positives for various performance checks that
share the same const usage analysis.

Path by Felix Berger

Reviewed By: njames93, PiotrZSL

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

diff --git a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
index 8915299..2d73179 100644 (file)
--- a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++ b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
@@ -43,14 +43,19 @@ constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt,
                            ASTContext &Context) {
   auto DeclRefToVar =
       declRefExpr(to(varDecl(equalsNode(&VarDecl)))).bind("declRef");
+  auto MemberExprOfVar = memberExpr(hasObjectExpression(DeclRefToVar));
+  auto DeclRefToVarOrMemberExprOfVar =
+      stmt(anyOf(DeclRefToVar, MemberExprOfVar));
   auto ConstMethodCallee = callee(cxxMethodDecl(isConst()));
   // Match method call expressions where the variable is referenced as the this
   // implicit object argument and operator call expression for member operators
   // where the variable is the 0-th argument.
   auto Matches = match(
-      findAll(expr(anyOf(cxxMemberCallExpr(ConstMethodCallee, on(DeclRefToVar)),
-                         cxxOperatorCallExpr(ConstMethodCallee,
-                                             hasArgument(0, DeclRefToVar))))),
+      findAll(expr(anyOf(
+          cxxMemberCallExpr(ConstMethodCallee,
+                            on(DeclRefToVarOrMemberExprOfVar)),
+          cxxOperatorCallExpr(ConstMethodCallee,
+                              hasArgument(0, DeclRefToVarOrMemberExprOfVar))))),
       Stmt, Context);
   SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
@@ -62,22 +67,23 @@ constReferenceDeclRefExprs(const VarDecl &VarDecl, const Stmt &Stmt,
       ConstReferenceOrValue,
       substTemplateTypeParmType(hasReplacementType(ConstReferenceOrValue))));
   auto UsedAsConstRefOrValueArg = forEachArgumentWithParam(
-      DeclRefToVar, parmVarDecl(hasType(ConstReferenceOrValueOrReplaced)));
+      DeclRefToVarOrMemberExprOfVar,
+      parmVarDecl(hasType(ConstReferenceOrValueOrReplaced)));
   Matches = match(findAll(invocation(UsedAsConstRefOrValueArg)), Stmt, Context);
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   // References and pointers to const assignments.
-  Matches =
-      match(findAll(declStmt(
-                has(varDecl(hasType(qualType(matchers::isReferenceToConst())),
-                            hasInitializer(ignoringImpCasts(DeclRefToVar)))))),
-            Stmt, Context);
+  Matches = match(
+      findAll(declStmt(has(varDecl(
+          hasType(qualType(matchers::isReferenceToConst())),
+          hasInitializer(ignoringImpCasts(DeclRefToVarOrMemberExprOfVar)))))),
+      Stmt, Context);
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
-  Matches =
-      match(findAll(declStmt(has(varDecl(
-                hasType(qualType(matchers::isPointerToConst())),
-                hasInitializer(ignoringImpCasts(unaryOperator(
-                    hasOperatorName("&"), hasUnaryOperand(DeclRefToVar)))))))),
-            Stmt, Context);
+  Matches = match(findAll(declStmt(has(varDecl(
+                      hasType(qualType(matchers::isPointerToConst())),
+                      hasInitializer(ignoringImpCasts(unaryOperator(
+                          hasOperatorName("&"),
+                          hasUnaryOperand(DeclRefToVarOrMemberExprOfVar)))))))),
+                  Stmt, Context);
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   return DeclRefs;
 }

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 2cc0010..53b987c 100644 (file)
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -432,6 +432,14 @@ Changes in existing checks
   `IgnoreTemplateInstantiations` option to optionally ignore virtual function
   overrides that are part of template instantiations.
 
+- Improved :doc:`performance-for-range-copy
+  <clang-tidy/checks/performance/for-range-copy>`
+  check by extending const usage analysis to include the type's members.
+
+- Improved :doc:`performance-inefficient-vector-operation
+  <clang-tidy/checks/performance/inefficient-vector-operation>`
+  check by extending const usage analysis to include the type's members.
+
 - Improved :doc:`performance-move-const-arg
   <clang-tidy/checks/performance/move-const-arg>` check to warn when move
   special member functions are not available.
@@ -445,6 +453,14 @@ Changes in existing checks
   <clang-tidy/checks/performance/noexcept-move-constructor>` checker that was causing
   false-positives when the move constructor or move assign operator were defaulted.
 
+- Improved :doc:`performance-unnecessary-copy-initialization
+  <clang-tidy/checks/performance/unnecessary-copy-initialization>`
+  check by extending const usage analysis to include the type's members.
+
+- Improved :doc:`performance-unnecessary-value-param
+  <clang-tidy/checks/performance/unnecessary-value-param>`
+  check by extending const usage analysis to include the type's members.
+
 - Improved :doc:`readability-container-data-pointer
   <clang-tidy/checks/readability/container-data-pointer>` check with new
   `IgnoredContainers` option to ignore some containers.

diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
index 07b5116..1a2eedc 100644 (file)
--- a/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
@@ -296,3 +296,38 @@ void positiveValueIteratorUsedElseWhere() {
     // SS : createView(*ValueReturningIterator<S>())) {
   }
 }
+
+void positiveConstMemberExpr() {
+  struct Struct {
+    Mutable Member;
+  };
+  for (Struct SS : View<Iterator<Struct>>()) {
+    // CHECK-MESSAGES: [[@LINE-1]]:15: warning: loop variable is copied
+    // CHECK-FIXES: for (const Struct& SS : View<Iterator<Struct>>()) {
+    auto MemberCopy = SS.Member;
+    const auto &ConstRef = SS.Member;
+    bool b = SS.Member.constMethod();
+    use(SS.Member);
+    useByConstValue(SS.Member);
+    useByValue(SS.Member);
+  }
+}
+
+void negativeNonConstMemberExpr() {
+  struct Struct {
+    Mutable Member;
+  };
+  for (Struct SS : View<Iterator<Struct>>()) {
+    SS.Member.setBool(true);
+  }
+  for (Struct SS : View<Iterator<Struct>>()) {
+    SS.Member[1];
+  }
+  for (Struct SS : View<Iterator<Struct>>()) {
+    mutate(SS.Member);
+  }
+  for (Struct SS : View<Iterator<Struct>>()) {
+    mutate(&SS.Member);
+  }
+}
+

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 2491aab..049ba64 100644 (file)
--- 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
@@ -843,3 +843,36 @@ void positiveSingleTemplateType() {
 }
 
 void instantiatePositiveSingleTemplateType() { positiveSingleTemplateType<ExpensiveToCopyType>(); }
+
+struct Struct {
+  ExpensiveToCopyType Member;
+};
+
+void positiveConstMemberExpr() {
+  Struct Orig;
+  auto UC = Orig;
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'UC'
+  // CHECK-FIXES: const auto& UC = Orig;
+  const auto &ConstRef = UC.Member;
+  auto MemberCopy = UC.Member;
+  bool b = UC.Member.constMethod();
+  useByValue(UC.Member);
+  useAsConstReference(UC.Member);
+  useByValue(UC.Member);
+}
+
+void negativeNonConstMemberExpr() {
+  Struct Orig;
+  {
+    auto Copy = Orig;
+    Copy.Member.nonConstMethod();
+  }
+  {
+    auto Copy = Orig;
+    mutate(Copy.Member);
+  }
+  {
+    auto Copy = Orig;
+    mutate(&Copy.Member);
+  }
+}

diff --git a/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp b/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
index d47add4..a653b11 100644 (file)
--- a/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
+++ b/clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp
@@ -114,8 +114,8 @@ TEST(ConstReferenceDeclRefExprsTest, ConstValueVar) {
       (void)*⌖
       S copy1 = /*const*/target;
       S copy2(/*const*/target);
-      useInt(target.int_member);
-      useIntConstRef(target.int_member);
+      useInt(/*const*/target.int_member);
+      useIntConstRef(/*const*/target.int_member);
       useIntPtr(target.ptr_member);
       useIntConstPtr(&target.int_member);
     }
@@ -140,8 +140,8 @@ TEST(ConstReferenceDeclRefExprsTest, ConstRefVar) {
       (void)*⌖
       S copy1 = /*const*/target;
       S copy2(/*const*/target);
-      useInt(target.int_member);
-      useIntConstRef(target.int_member);
+      useInt(/*const*/target.int_member);
+      useIntConstRef(/*const*/target.int_member);
       useIntPtr(target.ptr_member);
       useIntConstPtr(&target.int_member);
     }
@@ -172,8 +172,8 @@ TEST(ConstReferenceDeclRefExprsTest, ValueVar) {
       (void)*⌖
       S copy1 = /*const*/target;
       S copy2(/*const*/target);
-      useInt(target.int_member);
-      useIntConstRef(target.int_member);
+      useInt(/*const*/target.int_member);
+      useIntConstRef(/*const*/target.int_member);
       useIntPtr(target.ptr_member);
       useIntConstPtr(&target.int_member);
     }
@@ -199,8 +199,8 @@ TEST(ConstReferenceDeclRefExprsTest, RefVar) {
       (void)*⌖
       S copy1 = /*const*/target;
       S copy2(/*const*/target);
-      useInt(target.int_member);
-      useIntConstRef(target.int_member);
+      useInt(/*const*/target.int_member);
+      useIntConstRef(/*const*/target.int_member);
       useIntPtr(target.ptr_member);
       useIntConstPtr(&target.int_member);
     }