[clang-tidy] Fixup bugprone-loop-variable-copied-then-modified triggers in Blink. [chromium/src : main]
Daniel Cheng (Gerrit)
Daniel Cheng added 2 comments![]( "Open in Gerrit")
for (const auto& ch : chars) {I feel like the heuristic needs some tuning... this is definitely an example where `ch` was not mutated. Do you know if this is being classified as "unknown, possibly mtuated" or "definitely mutated"?
There are a couple other instances, but I consider those more harmless, e.g. the Rect change above in `visible_units.cc`. But I think people will (reasonably) complain about something like this.
Then again, maybe most instances of this pattern don't trigger the warning? It'd be nice to figure out what's different here though.
for (wtf_size_t i = 0; const auto& ac : chars) {Similarly here... this looks like an instance where it's not actually being mutated.
Related details
- Julia Hansbrough
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Julia Hansbrough (Gerrit)
Julia Hansbrough added 1 comment![]( "Open in Gerrit")
for (const auto& ch : chars) {I feel like the heuristic needs some tuning... this is definitely an example where `ch` was not mutated. Do you know if this is being classified as "unknown, possibly mtuated" or "definitely mutated"?
There are a couple other instances, but I consider those more harmless, e.g. the Rect change above in `visible_units.cc`. But I think people will (reasonably) complain about something like this.
Then again, maybe most instances of this pattern don't trigger the warning? It'd be nice to figure out what's different here though.
Yeah, so: the heuristic definitely tends toward marking *anything* it analyzes as a *potential* mutation as suspect (it's how LLVM's ExprMutationAnalyzer works). In the majority of cases the reason it marks things as such is fairly straightforward (if the variable is passed as an argument to some function, it's not able to introspect that function and assumes a mutation has occurred; likewise if some function is called on the object itself).
In this case, the analyzer is bothered by that `IsHTMLSpecialWhitespace` function—it *could* me modifying that value and so it flags it.
The hope is to reset Chromium to a "clean slate" with a one-time fixup—so that this alert doesn't trigger on anything in the codebase. Then, going forward, we can make a call on whether the check is overall too low-signal to be worth it, or, if it's worth it enough to update the style guide to "prefer const ref" (which would mute the alert even in cases where there isn't technically any bug) plus enable it as a regular clang-tidy check.
Let me know if that makes sense / sounds like an okay plan?
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Julia Hansbrough (Gerrit)
Julia Hansbrough added 1 comment![]( "Open in Gerrit")
for (const auto& ch : chars) {Julia HansbroughI feel like the heuristic needs some tuning... this is definitely an example where `ch` was not mutated. Do you know if this is being classified as "unknown, possibly mtuated" or "definitely mutated"?
There are a couple other instances, but I consider those more harmless, e.g. the Rect change above in `visible_units.cc`. But I think people will (reasonably) complain about something like this.
Then again, maybe most instances of this pattern don't trigger the warning? It'd be nice to figure out what's different here though.
Yeah, so: the heuristic definitely tends toward marking *anything* it analyzes as a *potential* mutation as suspect (it's how LLVM's ExprMutationAnalyzer works). In the majority of cases the reason it marks things as such is fairly straightforward (if the variable is passed as an argument to some function, it's not able to introspect that function and assumes a mutation has occurred; likewise if some function is called on the object itself).
In this case, the analyzer is bothered by that `IsHTMLSpecialWhitespace` function—it *could* me modifying that value and so it flags it.
The hope is to reset Chromium to a "clean slate" with a one-time fixup—so that this alert doesn't trigger on anything in the codebase. Then, going forward, we can make a call on whether the check is overall too low-signal to be worth it, or, if it's worth it enough to update the style guide to "prefer const ref" (which would mute the alert even in cases where there isn't technically any bug) plus enable it as a regular clang-tidy check.
Let me know if that makes sense / sounds like an okay plan?
Julia Hansbrough (Gerrit)
Julia Hansbrough abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |