[PATCH] D17064: Only invoke ForRangeCopyCheck on expensive-to-copy types.

[Felix Berger via cfe-commits]

flx created this revision.
flx added a reviewer: alexfh.
flx added a subscriber: cfe-commits.
flx set the repository for this revision to rL LLVM.

Fix oversight not checking the value of the Optional<bool> returned by 
isExpensiveToCopy().

Repository:
  rL LLVM

http://reviews.llvm.org/D17064

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
@@ -127,6 +127,7 @@
 }
 
 void use(const Mutable &M);
+void use(int I);
 void useTwice(const Mutable &M1, const Mutable &M2);
 void useByValue(Mutable M);
 void useByConstValue(const Mutable M);
@@ -170,6 +171,30 @@
   }
 }
 
+void negativeConstCheapToCopy() {
+  for (const int I : View<Iterator<int>>()) {
+  }
+}
+
+void negativeConstCheapToCopyTypedef() {
+  typedef const int ConstInt;
+  for (ConstInt C  : View<Iterator<ConstInt>>()) {
+  }
+}
+
+void negativeCheapToCopy() {
+  for (int I : View<Iterator<int>>()) {
+    use(I);
+  }
+}
+
+void negativeCheapToCopyTypedef() {
+  typedef int Int;
+  for (Int I : View<Iterator<Int>>()) {
+    use(I);
+  }
+}
+
 void positiveOnlyConstMethodInvoked() {
   for (auto M : 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
@@ -135,7 +135,9 @@
   } else if (!LoopVar.getType().isConstQualified()) {
     return false;
   }
-  if (!type_traits::isExpensiveToCopy(LoopVar.getType(), Context))
+  llvm::Optional<bool> Expensive =
+      type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
+  if (!Expensive || !*Expensive)
     return false;
   auto Diagnostic =
       diag(LoopVar.getLocation(),
@@ -150,8 +152,9 @@
 bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
     const VarDecl &LoopVar, const CXXForRangeStmt &ForRange,
     ASTContext &Context) {
-  if (LoopVar.getType().isConstQualified() ||
-      !type_traits::isExpensiveToCopy(LoopVar.getType(), Context)) {
+  llvm::Optional<bool> Expensive =
+      type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
+  if (LoopVar.getType().isConstQualified() || !Expensive || !*Expensive) {
     return false;
   }
   // Collect all DeclRefExprs to the loop variable and all CallExprs and



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
@@ -127,6 +127,7 @@
 }
 
 void use(const Mutable &M);
+void use(int I);
 void useTwice(const Mutable &M1, const Mutable &M2);
 void useByValue(Mutable M);
 void useByConstValue(const Mutable M);
@@ -170,6 +171,30 @@
   }
 }
 
+void negativeConstCheapToCopy() {
+  for (const int I : View<Iterator<int>>()) {
+  }
+}
+
+void negativeConstCheapToCopyTypedef() {
+  typedef const int ConstInt;
+  for (ConstInt C  : View<Iterator<ConstInt>>()) {
+  }
+}
+
+void negativeCheapToCopy() {
+  for (int I : View<Iterator<int>>()) {
+    use(I);
+  }
+}
+
+void negativeCheapToCopyTypedef() {
+  typedef int Int;
+  for (Int I : View<Iterator<Int>>()) {
+    use(I);
+  }
+}
+
 void positiveOnlyConstMethodInvoked() {
   for (auto M : 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
@@ -135,7 +135,9 @@
   } else if (!LoopVar.getType().isConstQualified()) {
     return false;
   }
-  if (!type_traits::isExpensiveToCopy(LoopVar.getType(), Context))
+  llvm::Optional<bool> Expensive =
+      type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
+  if (!Expensive || !*Expensive)
     return false;
   auto Diagnostic =
       diag(LoopVar.getLocation(),
@@ -150,8 +152,9 @@
 bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
     const VarDecl &LoopVar, const CXXForRangeStmt &ForRange,
     ASTContext &Context) {
-  if (LoopVar.getType().isConstQualified() ||
-      !type_traits::isExpensiveToCopy(LoopVar.getType(), Context)) {
+  llvm::Optional<bool> Expensive =
+      type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
+  if (LoopVar.getType().isConstQualified() || !Expensive || !*Expensive) {
     return false;
   }
   // Collect all DeclRefExprs to the loop variable and all CallExprs and

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