flowerhack wrote: @nicovank : Thanks for the thoughtful and detailed comments & review. Replying to your toplevel comment here:
> Suggesting const when the variable is modified in the loop always produces > invalid code (this is the case for the highlighted example in the > documentation): suggesting const makes little sense when we know/assume the > variable is modified. I performed an experiment where I ran this check over the Chromium codebase. After filtering out uninteresting code (`*_test.cc` files, etc), it returned about 200 results. Of those results, I picked about 50 to try handling by applying the naive fix—"if I switch this to a const ref, does it still compile?"—and in 70% of cases, the naive fix compiled without incident. (I believe that the disparity between "things that the ExprMutationAnalyzer think represents a mutation" vs "things that the `const` designation thinks represents a mutation" is due to the ExprMutationAnalyzer's more conservative semantics.) The remaining cases, to my eye, seemed like complicated enough situations that they would in fact benefit from more thought/care from the programmer, and it seems fine that the fix-it won't work out-of-the-box for those situations (generally require a judgment call from the programmer to either switch the variable in the loop to a non-const reference, or make the copy explicit in the loop). So, my hope with the fix-it would be to offer a simple suggestion that would work "most" of the time to minimize disruption to developers. That does mean some fixes suggested by this will not necessarily be bugfixes but rather a style enforcement—"prefer const references or references over copies in loop variables"— and that does make this something of an opinionated check, to be sure. And the `IgnoreInexpensiveVariables` toggle does exist for those who'd like some benefit from this check with a higher signal-to-noise ratio. But I do think it's valuable to have a version of the check that's concerned about *all* variables, since we've observed errors of this type involving inexpensive variables. > One idea maybe is to check if the copy is then used after modification. If it > is not, that's a strong signal the copy was unintended. But this seems tricky. I think such a modification would fail to catch many of the cases we care about, unfortunately. A pretty common pattern is to iterate over some set of objects (performing a copy on each iteration), modify a field in each object, and then continue. In this case the field isn't "used" again inside the body of that function. > One idea maybe is to check if the copy is then used after modification. If it > is not, that's a strong signal the copy was unintended. But this seems tricky. I think such a modification would fail to catch many of the cases we care about, unfortunately. A pretty common pattern is to iterate over some set of objects (performing a copy on each iteration), modify a field in each object, and then continue. In this case the field isn't "used" again inside the body of that function. https://github.com/llvm/llvm-project/pull/157213 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
