Use ExprMutationAnalyzer in performance-for-range-copy

[shuaiwang]

Summary

This gives better coverage to the check as ExprMutationAnalyzer is more
accurate comparing to isOnlyUsedAsConst.

Majority of wins come from const usage of member field, e.g.:

for (auto widget : container) { // copy of loop variable
  if (widget.type == BUTTON) { // const usage only recognized by ExprMutationAnalyzer
    // ...
  }
}

[JonasToth]

test/clang-tidy/performance-for-range-copy.cpp

120 ↗

(On Diff #153807)

I feel that const methods should be added as a test as well.

[hokein]

LGTM.

[shuaiwang]

test/clang-tidy/performance-for-range-copy.cpp

120 ↗

(On Diff #153807)

const methods are already tested with existing test cases.

clang-tools-extra/trunk/clang-tidy/performance/ForRangeCopyCheck.cpp

 //===--- ForRangeCopyCheck.cpp - clang-tidy--------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ForRangeCopyCheck.h"
-#include "../utils/DeclRefExprUtils.h"
+#include "../utils/ExprMutationAnalyzer.h"
 #include "../utils/FixItHintUtils.h"
 #include "../utils/TypeTraits.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace performance {

 
 bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
     const VarDecl &LoopVar, const CXXForRangeStmt &ForRange,
     ASTContext &Context) {
   llvm::Optional<bool> Expensive =
       utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
   if (LoopVar.getType().isConstQualified() || !Expensive || !*Expensive)
     return false;
-  if (!utils::decl_ref_expr::isOnlyUsedAsConst(LoopVar, *ForRange.getBody(),
-                                               Context))
+  if (utils::ExprMutationAnalyzer(ForRange.getBody(), &Context)
+          .isMutated(&LoopVar))
     return false;
   diag(LoopVar.getLocation(),
        "loop variable is copied but only used as const reference; consider "
        "making it a const reference")
       << utils::fixit::changeVarDeclToConst(LoopVar)
       << utils::fixit::changeVarDeclToReference(LoopVar, Context);
   return true;
 }
 
 } // namespace performance
 } // namespace tidy
 } // namespace clang

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

Mutable& operator[](int I) {
     return *this;
   }
   bool operator==(const Mutable &Other) const {
     return true;
   }
   ~Mutable() {}
 };
 
+struct Point {
+  ~Point() {}
+  int x, y;
+};
+
 Mutable& operator<<(Mutable &Out, bool B) {
   Out.setBool(B);
   return Out;
 }
 
 bool operator!=(const Mutable& M1, const Mutable& M2) {
   return false;
 }
for (auto UsedAsConst : View<Iterator<Mutable>>()) {
     // CHECK-FIXES: for (const auto& UsedAsConst : View<Iterator<Mutable>>()) {
     use(UsedAsConst);
     useTwice(UsedAsConst, UsedAsConst);
     useByValue(UsedAsConst);
     useByConstValue(UsedAsConst);
   }
 }
 
+void positiveOnlyAccessedFieldAsConst() {
+  for (auto UsedAsConst : View<Iterator<Point>>()) {
+    // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
+    // CHECK-FIXES: for (const auto& UsedAsConst : View<Iterator<Point>>()) {
+    use(UsedAsConst.x);
+    use(UsedAsConst.y);
+  }
+}
+
 void positiveOnlyUsedInCopyConstructor() {
   for (auto A : View<Iterator<Mutable>>()) {
     // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
     // CHECK-FIXES: for (const auto& A : View<Iterator<Mutable>>()) {
     Mutable Copy = A;
     Mutable Copy2(A);
   }
 }