Skip to content

Commit

Permalink
[clang-tidy] Omit cases where loop variable is not used in loop body in
Browse files Browse the repository at this point in the history
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
  • Loading branch information
hokein committed Aug 10, 2018
1 parent 75a9543 commit 25efa0f
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 9 deletions.
31 changes: 22 additions & 9 deletions clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
Expand Up @@ -8,6 +8,7 @@
//===----------------------------------------------------------------------===//

#include "ForRangeCopyCheck.h"
#include "../utils/DeclRefExprUtils.h"
#include "../utils/ExprMutationAnalyzer.h"
#include "../utils/FixItHintUtils.h"
#include "../utils/TypeTraits.h"
Expand Down Expand Up @@ -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
Expand Down
Expand Up @@ -260,3 +260,8 @@ void PositiveConstNonMemberOperatorInvoked() {
bool result = ConstOperatorInvokee != Mutable();
}
}

void IgnoreLoopVariableNotUsedInLoopBody() {
for (auto _ : View<Iterator<S>>()) {
}
}

0 comments on commit 25efa0f

Please sign in to comment.