[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.
hokein added inline comments.
Comment at: clang-tidy/performance/ForRangeCopyCheck.cpp:39
+ auto WhitelistClassMatcher =
+ cxxRecordDecl(hasAnyName(SmallVector(
+ WhitelistClasses.begin(), WhitelistClasses.end(;
JonasToth wrote:
> 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)
no needed for this patch. But yes! Moving to utility is reasonable to me.
Comment at: clang-tidy/performance/ForRangeCopyCheck.cpp:93
return false;
if (utils::ExprMutationAnalyzer(ForRange.getBody(), )
.isMutated())
JonasToth wrote:
> Adding a `..IsMutated() && IsReferenced(ForScope, LoopVar))` here
> should fix the warning as well.
I think ignoring `for (auto _ : state)` is a sensible solution. Thanks!
Comment at: test/clang-tidy/performance-for-range-copy.cpp:1
-// RUN: %check_clang_tidy %s performance-for-range-copy %t -- -- -std=c++11
-fno-delayed-template-parsing
+// RUN: %check_clang_tidy %s performance-for-range-copy %t
-config="{CheckOptions: [{key: "performance-for-range-copy.WhitelistClasses",
value: "WhitelistFoo"}]}" -- -std=c++11 -fno-delayed-template-parsing
JonasToth wrote:
> 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.
no needed this configuration now.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.
hokein updated this revision to Diff 159919.
hokein marked an inline comment as done.
hokein added a comment.
Adress review comment - ignore the case in the check.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50447
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
@@ -260,3 +260,8 @@
bool result = ConstOperatorInvokee != Mutable();
}
}
+
+void IgnoreLoopVariableNotUsedInLoopBody() {
+ for (auto _ : View>()) {
+ }
+}
Index: clang-tidy/performance/ForRangeCopyCheck.cpp
===
--- clang-tidy/performance/ForRangeCopyCheck.cpp
+++ 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 @@
utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
if (LoopVar.getType().isConstQualified() || !Expensive || !*Expensive)
return false;
- if (utils::ExprMutationAnalyzer(ForRange.getBody(), )
- .isMutated())
-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(), )
+ .isMutated() &&
+ !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
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
@@ -260,3 +260,8 @@
bool result = ConstOperatorInvokee != Mutable();
}
}
+
+void IgnoreLoopVariableNotUsedInLoopBody() {
+ for (auto _ : View>()) {
+ }
+}
Index: clang-tidy/performance/ForRangeCopyCheck.cpp
===
--- clang-tidy/performance/ForRangeCopyCheck.cpp
+++ 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 @@
utils::type_traits::isExpensiveToCopy(LoopVar.getType(), Context);
if (LoopVar.getType().isConstQualified() || !Expensive || !*Expensive)
return false;
- if (utils::ExprMutationAnalyzer(ForRange.getBody(), )
- .isMutated())
-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(), )
+ .isMutated() &&
+ !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)
+<<
[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.
shuaiwang added a comment. In https://reviews.llvm.org/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. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50447 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.
JonasToth added a comment.
> 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
> }
I see.
Comment at: clang-tidy/performance/ForRangeCopyCheck.cpp:93
return false;
if (utils::ExprMutationAnalyzer(ForRange.getBody(), )
.isMutated())
Adding a `..IsMutated() && IsReferenced(ForScope, LoopVar))` here
should fix the warning as well.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.
JonasToth added a comment.
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
>
> struct __attribute__((unused)) Foo {
> };
>
> void f() {
> std::vector 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
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.
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
struct __attribute__((unused)) Foo {
};
void f() {
std::vector 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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.
JonasToth added a comment. 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. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50447 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.
lebedev.ri added a subscriber: EricWF.
lebedev.ri added a comment.
In https://reviews.llvm.org/D50447#1192316, @hokein wrote:
> In https://reviews.llvm.org/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?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.
hokein added a comment.
In https://reviews.llvm.org/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
}
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.
JonasToth added inline comments.
Comment at: test/clang-tidy/performance-for-range-copy.cpp:1
-// RUN: %check_clang_tidy %s performance-for-range-copy %t -- -- -std=c++11
-fno-delayed-template-parsing
+// RUN: %check_clang_tidy %s performance-for-range-copy %t
-config="{CheckOptions: [{key: "performance-for-range-copy.WhitelistClasses",
value: "WhitelistFoo"}]}" -- -std=c++11 -fno-delayed-template-parsing
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.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.
JonasToth added a comment. 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. Comment at: clang-tidy/performance/ForRangeCopyCheck.cpp:39 + auto WhitelistClassMatcher = + cxxRecordDecl(hasAnyName(SmallVector( + WhitelistClasses.begin(), WhitelistClasses.end(; 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) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50447 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.
lebedev.ri added subscribers: shuaiwang, JonasToth, lebedev.ri. lebedev.ri added a comment. Regression is very broad term. Is it a false-positive? Perhaps it is better to address the issue in the ExprMutationAnalyzer itself? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50447 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.
hokein created this revision.
hokein added reviewers: ilya-biryukov, alexfh.
Herald added a subscriber: xazax.hun.
The upstream change r336737 causes a regression issue in our internal
base codebase.
Adding an option to the check allowing us whitelist these classes.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50447
Files:
clang-tidy/performance/ForRangeCopyCheck.cpp
clang-tidy/performance/ForRangeCopyCheck.h
docs/clang-tidy/checks/performance-for-range-copy.rst
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
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s performance-for-range-copy %t -- -- -std=c++11
-fno-delayed-template-parsing
+// RUN: %check_clang_tidy %s performance-for-range-copy %t
-config="{CheckOptions: [{key: "performance-for-range-copy.WhitelistClasses",
value: "WhitelistFoo"}]}" -- -std=c++11 -fno-delayed-template-parsing
namespace std {
@@ -260,3 +260,11 @@
bool result = ConstOperatorInvokee != Mutable();
}
}
+
+struct WhitelistFoo {
+ ~WhitelistFoo();
+};
+void IgnoreBlacklistedType() {
+ for (auto _ : View>()) {
+ }
+}
Index: docs/clang-tidy/checks/performance-for-range-copy.rst
===
--- docs/clang-tidy/checks/performance-for-range-copy.rst
+++ docs/clang-tidy/checks/performance-for-range-copy.rst
@@ -25,3 +25,8 @@
When non-zero, warns on any use of `auto` as the type of the range-based for
loop variable. Default is `0`.
+
+.. option:: WhitelistClasses
+
+ A semicolon-separated list of names of whitelist classes, which will be
+ ignored by the check. Default is empty.
Index: clang-tidy/performance/ForRangeCopyCheck.h
===
--- clang-tidy/performance/ForRangeCopyCheck.h
+++ clang-tidy/performance/ForRangeCopyCheck.h
@@ -40,6 +40,7 @@
ASTContext );
const bool WarnOnAllAutoCopies;
+ std::vector WhitelistClasses;
};
} // namespace performance
Index: clang-tidy/performance/ForRangeCopyCheck.cpp
===
--- clang-tidy/performance/ForRangeCopyCheck.cpp
+++ clang-tidy/performance/ForRangeCopyCheck.cpp
@@ -11,6 +11,7 @@
#include "../utils/ExprMutationAnalyzer.h"
#include "../utils/FixItHintUtils.h"
#include "../utils/TypeTraits.h"
+#include "../utils/OptionsUtils.h"
using namespace clang::ast_matchers;
@@ -20,18 +21,28 @@
ForRangeCopyCheck::ForRangeCopyCheck(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
- WarnOnAllAutoCopies(Options.get("WarnOnAllAutoCopies", 0)) {}
+ WarnOnAllAutoCopies(Options.get("WarnOnAllAutoCopies", 0)),
+ WhitelistClasses(utils::options::parseStringList(
+ Options.get("WhitelistClasses", ""))) {}
void ForRangeCopyCheck::storeOptions(ClangTidyOptions::OptionMap ) {
Options.store(Opts, "WarnOnAllAutoCopies", WarnOnAllAutoCopies);
+ Options.store(Opts, "WhitelistClasses",
+utils::options::serializeStringList(WhitelistClasses));
}
void ForRangeCopyCheck::registerMatchers(MatchFinder *Finder) {
// Match loop variables that are not references or pointers or are already
// initialized through MaterializeTemporaryExpr which indicates a type
// conversion.
+ auto WhitelistClassMatcher =
+ cxxRecordDecl(hasAnyName(SmallVector(
+ WhitelistClasses.begin(), WhitelistClasses.end(;
+ auto WhitelistType = hasUnqualifiedDesugaredType(
+ recordType(hasDeclaration(WhitelistClassMatcher)));
auto LoopVar = varDecl(
- hasType(hasCanonicalType(unless(anyOf(referenceType(), pointerType(),
+ hasType(hasCanonicalType(
+ unless(anyOf(referenceType(), pointerType(), WhitelistType,
unless(hasInitializer(expr(hasDescendant(materializeTemporaryExpr());
Finder->addMatcher(cxxForRangeStmt(hasLoopVariable(LoopVar.bind("loopVar")))
.bind("forRange"),
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
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s performance-for-range-copy %t -- -- -std=c++11 -fno-delayed-template-parsing
+// RUN: %check_clang_tidy %s performance-for-range-copy %t -config="{CheckOptions: [{key: "performance-for-range-copy.WhitelistClasses", value: "WhitelistFoo"}]}" -- -std=c++11 -fno-delayed-template-parsing
namespace std {
@@ -260,3 +260,11 @@
bool result = ConstOperatorInvokee != Mutable();
}
}
+
+struct WhitelistFoo {
+ ~WhitelistFoo();
+};
+void IgnoreBlacklistedType() {
+ for (auto _ :
