[PATCH] D48854: Use ExprMutationAnalyzer in performance-for-range-copy

[Shuai Wang via Phabricator via cfe-commits]

shuaiwang created this revision.
Herald added a reviewer: george.karpenkov.
Herald added subscribers: cfe-commits, a.sidorin.

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
    // ...
  }

}


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48854

Files:
  clang-tidy/performance/ForRangeCopyCheck.cpp
  test/clang-tidy/performance-for-range-copy.cpp


Index: test/clang-tidy/performance-for-range-copy.cpp
===================================================================
--- test/clang-tidy/performance-for-range-copy.cpp
+++ test/clang-tidy/performance-for-range-copy.cpp
@@ -117,6 +117,11 @@
   ~Mutable() {}
 };
 
+struct Point {
+  ~Point() {}
+  int x, y;
+};
+
 Mutable& operator<<(Mutable &Out, bool B) {
   Out.setBool(B);
   return Out;
@@ -214,6 +219,15 @@
   }
 }
 
+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]
Index: clang-tidy/performance/ForRangeCopyCheck.cpp
===================================================================
--- clang-tidy/performance/ForRangeCopyCheck.cpp
+++ clang-tidy/performance/ForRangeCopyCheck.cpp
@@ -8,7 +8,7 @@
 
//===----------------------------------------------------------------------===//
 
 #include "ForRangeCopyCheck.h"
-#include "../utils/DeclRefExprUtils.h"
+#include "../utils/ExprMutationAnalyzer.h"
 #include "../utils/FixItHintUtils.h"
 #include "../utils/TypeTraits.h"
 
@@ -79,8 +79,8 @@
       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 "



Index: test/clang-tidy/performance-for-range-copy.cpp
===================================================================
--- test/clang-tidy/performance-for-range-copy.cpp
+++ test/clang-tidy/performance-for-range-copy.cpp
@@ -117,6 +117,11 @@
   ~Mutable() {}
 };
 
+struct Point {
+  ~Point() {}
+  int x, y;
+};
+
 Mutable& operator<<(Mutable &Out, bool B) {
   Out.setBool(B);
   return Out;
@@ -214,6 +219,15 @@
   }
 }
 
+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]
Index: clang-tidy/performance/ForRangeCopyCheck.cpp
===================================================================
--- clang-tidy/performance/ForRangeCopyCheck.cpp
+++ clang-tidy/performance/ForRangeCopyCheck.cpp
@@ -8,7 +8,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "ForRangeCopyCheck.h"
-#include "../utils/DeclRefExprUtils.h"
+#include "../utils/ExprMutationAnalyzer.h"
 #include "../utils/FixItHintUtils.h"
 #include "../utils/TypeTraits.h"
 
@@ -79,8 +79,8 @@
       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 "

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits