[clang-tidy] Omit cases where loop variable is not used in loop body in performance-for-range-copy.

[hokein]

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 like below, changing to const auto _ will introduce an "unused" complier warning.

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

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

[lebedev.ri]

Regression is very broad term. Is it a false-positive?
Perhaps it is better to address the issue in the ExprMutationAnalyzer itself?

[JonasToth]

If whitelisting works, thats fine. But I agree with @lebedev.ri that a contribution/improvement to the ExprMutAnalyzer is the better option. This is especially the case, because multiple checks (and maybe even some other parts then clang-tidy) will utilize this analysis.

clang-tidy/performance/ForRangeCopyCheck.cpp

39 ↗

(On Diff #159707)

I have seens this pattern now multiple times in various checks, we should have some utility wrapping the hasAnyName(MyList). (Just in general, does not need to happen with this check)

[JonasToth]

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

1 ↗

(On Diff #159707)

I would prefer a single file, that has the configuration and leave the standard test like it is.

with this, you can always track what is actually done by default and what is done with different conigurations + potential inconsistencies that might occur by bugs in the configured code.

[hokein]

In D50447#1192280, @JonasToth wrote:

If whitelisting works, thats fine. But I agree with @lebedev.ri that a contribution/improvement to the ExprMutAnalyzer is the better option. This is especially the case, because multiple checks (and maybe even some other parts then clang-tidy) will utilize this analysis.

I'm sorry for not explaining it with more details. Might be "regression" is a confusing world :(. It is not false positive. The change using ExprMutAnalyzer is reasonable, IMO. It makes this check smarter to catch cases which will not be caught before. For example,

for (auto widget : container) {
  widget.call_const_method(); // const usage recongized by ExprMutAnalyzer, so it is fine to change widget to `const auto&`
}

But in our codebase, we have code intended to use like below, and it is in base library, and is used widely. I think it makes sense to whitelist this class in our internal configuration.

for (auto _ : state) {
   ... // no `_` being referenced in the for-loop body
}

[lebedev.ri]

In D50447#1192316, @hokein wrote:

In D50447#1192280, @JonasToth wrote:

If whitelisting works, thats fine. But I agree with @lebedev.ri that a contribution/improvement to the ExprMutAnalyzer is the better option. This is especially the case, because multiple checks (and maybe even some other parts then clang-tidy) will utilize this analysis.

I'm sorry for not explaining it with more details. Might be "regression" is a confusing world :(. It is not false positive. The change using ExprMutAnalyzer is reasonable, IMO. It makes this check smarter to catch cases which will not be caught before. For example,

for (auto widget : container) {
  widget.call_const_method(); // const usage recongized by ExprMutAnalyzer, so it is fine to change widget to `const auto&`
}

But in our codebase, we have code intended to use like below, and it is in base library, and is used widely. I think it makes sense to whitelist this class in our internal configuration.

for (auto _ : state) {
   ... // no `_` being referenced in the for-loop body
}

That looks remarkably like google benchmark main loop. (i don't know at hand if making this const will break it, but probably not?)
I wonder if instead there should be an option to not complain about the variables that aren't actually used?

[JonasToth]

Is the type important, or specifics about the variable (e.g. the name?)
The _ variable names are sometimes used for RAII variables/lambdas
that shall just do some cleanup, e.g. gsl::finally().

It might make sense to give ExprMutAnalyzer an ignore-mechanism.

I wonder if instead there should be an option to not complain about the variables that aren't actually used?

Checking for variable usage would be simple. The ExprMutAnalyzer
always analyses the scope, e.g. a function. You can match this scope for
a DeclRefExpr of the variable, empty result means no usage.

[hokein]

That looks remarkably like google benchmark main loop. (i don't know at hand if making this const will break it, but probably not?)
I wonder if instead there should be an option to not complain about the variables that aren't actually used?

Yeah, it is google benchmark library.

The new fix for (const auto& _ : state) will trigger -Wunused warning. __attribute__((unused)) doesn't help to suppress the warning :(

$ cat /tmp/main4.cc 
#include <vector>

struct __attribute__((unused)) Foo {
};

void f() {
  std::vector<Foo> foos;
  for (const Foo& _ : foos) {
  }
}
$ g++ /tmp/main4.cc -Wunused                                                                                                                                                                                 
/tmp/main4.cc: In function ‘void f()’:
/tmp/main4.cc:8:19: warning: unused variable ‘_’ [-Wunused-variable]
   for (const Foo& _ : foos) {
                   ^

Is the type important, or specifics about the variable (e.g. the name?)
The _ variable names are sometimes used for RAII variables/lambdas
that shall just do some cleanup, e.g. gsl::finally().

It might make sense to give ExprMutAnalyzer an ignore-mechanism.

I wonder if instead there should be an option to not complain about the variables that aren't actually used?

Checking for variable usage would be simple. The ExprMutAnalyzer
always analyses the scope, e.g. a function. You can match this scope for
a DeclRefExpr of the variable, empty result means no usage.

Both type and variable name "_" can fix the issue here, but the variable name seems a fragile way (if the name is changed, things will be broken again).

Yeah, adding an ignore mechanism to ExprMutAnalyzer is another option.

[JonasToth]

Given it is about the performance in loops and the optimizer?! can
better work with the copy-and-don't-use it might make sense to just
check if the variable is dereferenced in the scope of the loop (any
declRefExpr exists).

That is more userfriendly, too.

Am 08.08.2018 um 17:02 schrieb Haojian Wu via Phabricator:

hokein added a comment.

That looks remarkably like google benchmark main loop. (i don't know at hand if making this const will break it, but probably not?)
I wonder if instead there should be an option to not complain about the variables that aren't actually used?

Yeah, it is google benchmark library.

The new fix for (const auto& _ : state) will trigger -Wunused warning. __attribute__((unused)) doesn't help to suppress the warning :(

$ cat /tmp/main4.cc 
#include <vector>

struct __attribute__((unused)) Foo {
};

void f() {
  std::vector<Foo> foos;
  for (const Foo& _ : foos) {
  }
}
$ g++ /tmp/main4.cc -Wunused                                                                                                                                                                                 
/tmp/main4.cc: In function ‘void f()’:
/tmp/main4.cc:8:19: warning: unused variable ‘_’ [-Wunused-variable]
   for (const Foo& _ : foos) {
                   ^

Is the type important, or specifics about the variable (e.g. the name?)
The _ variable names are sometimes used for RAII variables/lambdas
that shall just do some cleanup, e.g. gsl::finally().

It might make sense to give ExprMutAnalyzer an ignore-mechanism.

I wonder if instead there should be an option to not complain about the variables that aren't actually used?

Checking for variable usage would be simple. The ExprMutAnalyzer
always analyses the scope, e.g. a function. You can match this scope for
a DeclRefExpr of the variable, empty result means no usage.

Both type and variable name "_" can fix the issue here, but the variable name seems a fragile way (if the name is changed, things will be broken again).

Yeah, adding an ignore mechanism to ExprMutAnalyzer is another option.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D50447

[JonasToth]

This comment has been deleted.

clang-tidy/performance/ForRangeCopyCheck.cpp

93 ↗

(On Diff #159707)

Adding a ..IsMutated(&LoopVar) && IsReferenced(ForScope, LoopVar)) here should fix the warning as well.

[shuaiwang]

In D50447#1192393, @JonasToth wrote:

... just check if the variable is dereferenced in the scope of the loop (any
declRefExpr exists).

+1
And I would imagine it's very rare (as in categories not raw number of occurrences) for a loop variable to be not used in the loop body so it's probably a safe change. Though I'm interested to learn whether there's any real false negative by doing this.

[hokein]

Adress review comment - ignore the case in the check.

[hokein]

clang-tidy/performance/ForRangeCopyCheck.cpp

39 ↗

(On Diff #159707)

no needed for this patch. But yes! Moving to utility is reasonable to me.

93 ↗

(On Diff #159707)

I think ignoring for (auto _ : state) is a sensible solution. Thanks!

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

1 ↗

(On Diff #159707)

no needed this configuration now.

[JonasToth]

LGTM, but alex or aaron should allow the commit ;)

[alexfh]

LG

[lebedev.ri]

It seems this ended up silently being a catch-all, with no option to control this behavior, and i don't see any comments discussing this..

[JonasToth]

Do you think it is a bad idea? If the variable is not used it is ok to
ignore it in this particular circumstance. Other warnings/check should
deal with such a situation IMHO.

Am 10.08.2018 um 10:29 schrieb Roman Lebedev via Phabricator:

lebedev.ri added a comment.

It seems this ended up silently being a catch-all, with no option to control this behavior, and i don't see any comments discussing this..

Repository:

rL LLVM

https://reviews.llvm.org/D50447

[shuaiwang]

In D50447#1194967, @JonasToth wrote:

Do you think it is a bad idea? If the variable is not used it is ok to
ignore it in this particular circumstance. Other warnings/check should
deal with such a situation IMHO.

Am 10.08.2018 um 10:29 schrieb Roman Lebedev via Phabricator:

lebedev.ri added a comment.

It seems this ended up silently being a catch-all, with no option to control this behavior, and i don't see any comments discussing this..

Repository:

rL LLVM

https://reviews.llvm.org/D50447

Not sure whether hokein have done that already but I did run through our code base and AFAICT there's no false negative.

[JonasToth]

Could there even be a false positive? In a sense, the variable is never
used, so it does not matter, not?

Am 10.08.2018 um 22:17 schrieb Shuai Wang via Phabricator:

shuaiwang added a comment.

In https://reviews.llvm.org/D50447#1194967, @JonasToth wrote:

Do you think it is a bad idea? If the variable is not used it is ok to
ignore it in this particular circumstance. Other warnings/check should
deal with such a situation IMHO.

Am 10.08.2018 um 10:29 schrieb Roman Lebedev via Phabricator:

lebedev.ri added a comment.

It seems this ended up silently being a catch-all, with no option to control this behavior, and i don't see any comments discussing this..

Repository:

rL LLVM

https://reviews.llvm.org/D50447

Not sure whether hokein have done that already but I did run through our code base and AFAICT there's no false negative.

Repository:

rL LLVM

https://reviews.llvm.org/D50447

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 {

 
 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::ExprMutationAnalyzer(ForRange.getBody(), &Context)
-          .isMutated(&LoopVar))
-    return false;
+  // 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
 } // namespace tidy
 } // namespace clang

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

 
 void PositiveConstNonMemberOperatorInvoked() {
   for (auto ConstOperatorInvokee : 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& ConstOperatorInvokee : View<Iterator<Mutable>>()) {
     bool result = ConstOperatorInvokee != Mutable();
   }
 }
+
+void IgnoreLoopVariableNotUsedInLoopBody() {
+  for (auto _ : View<Iterator<S>>()) {
+  }
+}