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
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
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
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
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
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
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
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
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
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
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
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
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
13 matches
Mail list logo
