[clang-tidy] Omit cases where loop variable is not used in loop body in

performance-for-range-copy check.

Summary:
The upstream change r336737 make the check too smart to fix the case
where loop variable could be used as `const auto&`.

But for the case below, changing to `const auto _` will introduce
an unused complier warning.

```
for (auto _ : state) {
  // no references for _.
}
```

This patch omit this case, and it is safe to do it as the case is very rare.

Reviewers: ilya-biryukov, alexfh

Subscribers: xazax.hun, cfe-commits

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

llvm-svn: 339415

2 files changed, 27 insertions, 9 deletions

diff --git a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
index c8c90638255..867d95d8edd 100644
--- a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
@@ -8,6 +8,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "ForRangeCopyCheck.h"
+#include "../utils/DeclRefExprUtils.h"
 #include "../utils/ExprMutationAnalyzer.h"
 #include "../utils/FixItHintUtils.h"
 #include "../utils/TypeTraits.h"
@@ -79,15 +80,27 @@ bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
       utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
   if (LoopVar.getType().isConstQualified() || !Expensive || !*Expensive)
     return false;
-  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;
+  // We omit the case where the loop variable is not used in the loop body. E.g.
+  //
+  // for (auto _ : benchmark_state) {
+  // }
+  //
+  // Because the fix (changing to `const auto &`) will introduce an unused
+  // compiler warning which can't be suppressed.
+  // Since this case is very rare, it is safe to ignore it.
+  if (!utils::ExprMutationAnalyzer(ForRange.getBody(), &Context)
+          .isMutated(&LoopVar) &&
+      !utils::decl_ref_expr::allDeclRefExprs(LoopVar, *ForRange.getBody(),
+                                             Context)
+           .empty()) {
+    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;
+  }
+  return false;
 }
 
 } // namespace performance
diff --git a/clang-tools-extra/test/clang-tidy/performance-for-range-copy.cpp b/clang-tools-extra/test/clang-tidy/performance-for-range-copy.cpp
index 1c77996c6ca..8a43ac0c205 100644
--- a/clang-tools-extra/test/clang-tidy/performance-for-range-copy.cpp
+++ b/clang-tools-extra/test/clang-tidy/performance-for-range-copy.cpp
@@ -260,3 +260,8 @@ void PositiveConstNonMemberOperatorInvoked() {
     bool result = ConstOperatorInvokee != Mutable();
   }
 }
+
+void IgnoreLoopVariableNotUsedInLoopBody() {
+  for (auto _ : View<Iterator<S>>()) {
+  }
+}