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

Authored by hokein on Aug 8 2018, 6:44 AM.

Details

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.

Diff Detail

Repository
rL LLVM
hokein created this revision.Aug 8 2018, 6:44 AM

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

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 added inline comments.Aug 8 2018, 6:54 AM
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 added a comment.Aug 8 2018, 7:10 AM

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
}

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?

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 added a comment.Aug 8 2018, 8:02 AM

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.

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 added a comment.EditedAug 8 2018, 8:08 AM
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.

... 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 updated this revision to Diff 159919.Aug 9 2018, 7:02 AM
hokein marked an inline comment as done.

Adress review comment - ignore the case in the check.

hokein added inline comments.Aug 9 2018, 7:03 AM
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.

hokein retitled this revision from [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check. to [clang-tidy] Omit cases where loop variable is not used in loop body in performance-for-range-copy..Aug 9 2018, 7:09 AM
hokein edited the summary of this revision. (Show Details)

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

This revision is now accepted and ready to land.Aug 9 2018, 3:37 PM
This revision was automatically updated to reflect the committed changes.

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

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

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.

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