[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

aaronpuchert added a comment. I've been thinking about the warning messages, which seem to a bit inaccurate. Sorry for piggy-backing this onto the change, I hope you don't mind. Comment at: clang/lib/Sema/SemaStmt.cpp:2758-2759 // the correct time to bind a const referenc

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

Mordante added a comment. Created D73434 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73007/new/ https://reviews.llvm.org/D73007 ___ cfe-commits mailing list cfe-com

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

Mordante added a comment. @aaronpuchert, @aaron.ballman I agree with the proposal, I'll work on a patch. @hans Once the patch is accepted I'll send you a pull request and an update for the release notes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.or

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

aaron.ballman added a comment. In D73007#1839648 , @aaronpuchert wrote: > Here is a proposal: we add two children to `-Wrange-loop-analysis`. > > - `-Wrange-loop-construct` warns about possibly unintended constructor calls. > This might be in `-Wall`. It

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

aaronpuchert added a comment. Here is a proposal: we add two children to `-Wrange-loop-analysis`. - `-Wrange-loop-construct` warns about possibly unintended constructor calls. This might be in `-Wall`. It contains - `warn_for_range_copy`: loop variable A of type B creates a copy from type C

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

hans added a comment. In D73007#1837621 , @rtrieu wrote: > I'm in favor of splitting the warning into subgroups, then deciding which > ones should be in -Wall. We've done this with other warnings such as the > conversion warnings and tautological compar

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

aaron.ballman added a comment. In D73007#1837621 , @rtrieu wrote: > I'm in favor of splitting the warning into subgroups, then deciding which > ones should be in -Wall. We've done this with other warnings such as the > conversion warnings and tautologic

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

rtrieu added a comment. I'm in favor of splitting the warning into subgroups, then deciding which ones should be in -Wall. We've done this with other warnings such as the conversion warnings and tautological compare warnings, with only a subset of them in -Wall. Repository: rG LLVM Github M

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

aaronpuchert added a comment. In D73007#1837271 , @aaronpuchert wrote: > one could also argue that this is a bit harder to follow in a range-based for > loop. This seems to be the argumentation of https://bugs.llvm.org/show_bug.cgi?id=32823#c0, so I g

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

aaronpuchert added a comment. I think we can actually view this in more general terms, unrelated to range-based for loops. // X is non-trivially copyable. struct X { X(const X&); }; struct Y { Y(const X&); }; X retVal(); const X& retRef(); // In function scope ... const X &x1

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

hans added a comment. I've cherry-picked this to 10.x in 318677e78def0023d210a29f4b3cf648e02f9fdc . Please let me know if there are any follow-ups. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://re

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:2703-2704 // suggest the const reference type that would do so. // For instance, given "for (const &Foo : Range)", suggest // "for (const Foo : Range)" to denote a copy is made for the loop. If // p

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

aaronpuchert added a comment. I also still think that `warn_for_range_copy` //should be// emitted even in templates. These aren't false positives in my opinion, especially since we're now pretty conservative about that warning. Comment at: clang/lib/Sema/SemaStmt.cpp:2703-27

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

This revision was automatically updated to reflect the committed changes. Mordante marked an inline comment as done. Closed by commit rG41fcd17250fa: [Sema] Avoid Wrange-loop-analysis false positives (authored by Mordante). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

Mordante marked 2 inline comments as done. Mordante added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:2703-2704 // suggest the const reference type that would do so. // For instance, given "for (const &Foo : Range)", suggest // "for (const Foo : Range)" to denote

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

Mordante added a comment. In D73007#1829597 , @aaron.ballman wrote: > LGTM! Do you think this should also be pushed onto the release branch? Thanks for the review. Yes the bug has been marked as a release blocker. Repository: rG LLVM Github Monorepo

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

Mordante added a comment. In D73007#1829709 , @xbolva00 wrote: > Yes, but minimal fix is better for release branch, so @hans should merge it. > > Handling your case probably needs more work and patches.. Agreed. Repository: rG LLVM Github Monorepo C

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

aaronpuchert added a comment. In D73007#1829709 , @xbolva00 wrote: > Yes, but minimal fix is better for release branch, so @hans should merge it. I think `-Wall` shouldn't warn about reference types in a range-based for loop (unless it's the wrong type

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

xbolva00 added a subscriber: hans. xbolva00 added a comment. Yes, but minimal fix is better for release branch, so @hans should merge it. Handling your case probably needs more work and patches.. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73007/

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

aaronpuchert added a comment. As I wrote on the bug , I think we should only suppress one of the warnings in templates (and maybe always): :18:22: warning: loop variable '_' is always a copy because the range of type 'const Rng' does not return a r

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Do you think this should also be pushed onto the release branch? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73007/new/ ht

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 62002 tests passed, 0 failed and 783 were skipped. {icon question-circle color=gray} clang-tidy: unknown. {icon times-circle color=red} clang-format: fail. Please format your changes with clang-format by runnin

[PATCH] D73007: [Sema] Avoid Wrange-loop-analysis false positives

Mordante created this revision. Mordante added reviewers: rsmith, rtrieu, aaron.ballman, xbolva00. Mordante added a project: clang. When Wrange-loop-analysis issues a diagnostic on a dependent type in a template the diagnostic may not be valid for all instantiations. Therefore the diagnostic is