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

[xgupta]

Summary

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.

[flx]

Add tests for PerformanceUnnecssaryCopyInitialization check which actually uses
decl_ref_expr::isOnlyUsedAsConst().

[flx]

Hi,

could someone please take a look at this? Thanks!

[flx]

Could someone take a look at this change?

[flx]

Could someone take a look at this patch? Thank you!

[njames93]

LGTM, but with a very minor nit.

clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp

46

Can you use hasObjectExpression instead of has here, Its a bit more explicit in what we are matching.

[xgupta]

Rebase and address one last comment.

I think now it will be fine to commit?

[PiotrZSL]

Some release notes would be nice, looks like impacted checks are: performance-unnecessary-copy-initialization, performance-inefficient-vector-operation, performance-unnecessary-value-param, performance-for-range-copy. Some info about "Improved XYZ check by handling handling more properly const member expressions" or something similar. And wait for test results, I tried this change locally, and tests were failing.

[xgupta]

Added release note and fix unit test case

[PiotrZSL]

clang-tools-extra/docs/ReleaseNotes.rst

539

Split it multiple entry's, 1 per check, keep in alphabetical order.
and maybe keep it in format;
"Improved XYZ check by extending const usage analysis to include the type's members."

[PiotrZSL]

clang-tools-extra/docs/ReleaseNotes.rst

539

And if we already got entry for some check, just try adding this info to that.

[xgupta]

Update release note

[xgupta]

minor update

[PiotrZSL]

clang-tools-extra/docs/ReleaseNotes.rst

539

sort them by name, performance checks are before readability

[xgupta]

Address comment

[PiotrZSL]

LGTM

clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp

 
 // Finds all DeclRefExprs where a const method is called on VarDecl or VarDecl
 // is the a const reference or value argument to a CallExpr or CXXConstructExpr.
 SmallPtrSet<const DeclRefExpr *, 16>
 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)),
+                              hasArgument(0, DeclRefToVarOrMemberExprOfVar))))),
       Stmt, Context);
   SmallPtrSet<const DeclRefExpr *, 16> DeclRefs;
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   auto ConstReferenceOrValue =
       qualType(anyOf(matchers::isReferenceToConst(),
                      unless(anyOf(referenceType(), pointerType(),
                                   substTemplateTypeParmType()))));
   auto ConstReferenceOrValueOrReplaced = qualType(anyOf(
       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)))))),
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
-  Matches =
-      match(findAll(declStmt(has(varDecl(
+  Matches = match(findAll(declStmt(has(varDecl(
                       hasType(qualType(matchers::isPointerToConst())),
                       hasInitializer(ignoringImpCasts(unaryOperator(
-                    hasOperatorName("&"), hasUnaryOperand(DeclRefToVar)))))))),
+                          hasOperatorName("&"),
+                          hasUnaryOperand(DeclRefToVarOrMemberExprOfVar)))))))),
                   Stmt, Context);
   extractNodesByIdTo(Matches, "declRef", DeclRefs);
   return DeclRefs;
 }
 
 bool isOnlyUsedAsConst(const VarDecl &Var, const Stmt &Stmt,
                        ASTContext &Context) {
   // Collect all DeclRefExprs to the loop variable and all CallExprs and
   // CXXConstructExprs where the loop variable is used as argument to a const

clang-tools-extra/docs/ReleaseNotes.rst

- Fixed false positive in :doc:`modernize-use-equals-default
   functions containing macros or preprocessor directives, and out-of-line special
   member functions in unions.
 
 - Improved :doc:`modernize-use-override
   <clang-tidy/checks/modernize/use-override>` check with new
   `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.
 
 - Improved :doc:`performance-no-automatic-move
   <clang-tidy/checks/performance/no-automatic-move>` check to warn on
   ``const &&`` constructors and ignore ``const`` local variable to which NRVO
   is applied.
 
 - Fixed an issue in the :doc:`performance-noexcept-move-constructor
   <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.
 
 - Fixed a false positive in :doc:`readability-container-size-empty
   <clang-tidy/checks/readability/container-size-empty>` check when comparing
   ``std::array`` objects to default constructed ones. The behavior for this and
   other relevant classes can now be configured with a new option.
- Improved :doc:`readability-redundant-string-cstr
   arguments to ``std::print``, ``std::format`` or other functions listed in
   the ``StringParameterFunction`` check option.
 
 - Improved :doc:`readability-static-accessed-through-instance
   <clang-tidy/checks/readability/static-accessed-through-instance>` check to
   support unscoped enumerations through instances and fixed usage of anonymous
   structs or classes.
 
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to clang-include-fixer

clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp

 void positiveValueIteratorUsedElseWhere() {
   for (const S SS : createView(*ValueReturningIterator<S>())) {
     // CHECK-MESSAGES: [[@LINE-1]]:16: warning: the loop variable's type is not
     // a reference type; this creates a copy in each iteration; consider making
     // this a reference [performance-for-range-copy] CHECK-FIXES: for (const S&
     // 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);
+  }
+}
+

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

void positiveSingleTemplateType() {
 
   A UnnecessaryCopy3 = Orig.template templatedAccessor<A>();
   // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the variable 'UnnecessaryCopy3' is copy-constructed from a const reference
   // CHECK-FIXES: const A& UnnecessaryCopy3 = Orig.template templatedAccessor<A>();
   UnnecessaryCopy3.constMethod();
 }
 
 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);
+  }
+}

clang-tools-extra/unittests/clang-tidy/DeclRefExprUtilsTest.cpp

void f(const S target) {
       useConstRef((/*const*/target));
       (/*const*/target).constMethod();
       (void)(/*const*/target == /*const*/target);
       (void)target;
       (void)⌖
       (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);
     }
 )");
 }
 
 TEST(ConstReferenceDeclRefExprsTest, ConstRefVar) {
   RunTest(R"(
     void f(const S& target) {
       useVal(/*const*/target);
       useConstRef(/*const*/target);
       useConstPtr(&target);
       useConstPtrConstRef(&target);
       /*const*/target.constMethod();
       /*const*/target(ConstTag{});
       /*const*/target[42];
       useConstRef((/*const*/target));
       (/*const*/target).constMethod();
       (void)(/*const*/target == /*const*/target);
       (void)target;
       (void)⌖
       (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);
     }
 )");
 }
 
 TEST(ConstReferenceDeclRefExprsTest, ValueVar) {
   RunTest(R"(
void f(S target, const S& other) {
       (/*const*/target).constMethod();
       (void)(/*const*/target == /*const*/target);
       (void)(/*const*/target == other);
       (void)target;
       (void)⌖
       (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);
     }
 )");
 }
 
 TEST(ConstReferenceDeclRefExprsTest, RefVar) {
   RunTest(R"(
void f(S& target) {
       useConstRef((/*const*/target));
       (/*const*/target).constMethod();
       (void)(/*const*/target == /*const*/target);
       (void)target;
       (void)⌖
       (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);
     }
 )");
 }
 
 TEST(ConstReferenceDeclRefExprsTest, PtrVar) {
   RunTest(R"(