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 // possible, also suggest "for (const &Bar : Range)" if this type prevents ---------------- aaronpuchert wrote: > Mordante wrote: > > aaronpuchert wrote: > > > That's the part the bothers me. > > I think we shouldn't remove the warning, it is useful. We can consider to > > move this out of -Wall, but I first like to hear the opinion of the other > > reviewers in this matter. If we want this change I'll be happy to create a > > new patch. > I don't see a problem with having this as an optional warning that can be > turned on for those who want it. > > Note that this isn't my personal opinion, I'm actually fine with it. But I've > met considerable resistance to turning the warning on before, and it's > somewhat hard to argue against that. Binding references to temporaries is > perfectly normal C++, and we don't discourage it anywhere else. > I think we shouldn't remove the warning, it is useful. Agreed. > We can consider to move this out of -Wall, but I first like to hear the > opinion of the other reviewers in this matter. If we want this change I'll be > happy to create a new patch. We typically want warnings to be enabled by default and have a low-false positive rate so that users get the benefit of the diagnostic without undue effort (like manually disabling it because they find the diagnostics low-value). I think the point @aaronpuchert raises is an interesting one in that binding a reference to a temp is not necessarily wrong, so I could see some users wanting to disable the diagnostic rather than change their code, which suggests that -Wall may not be the best place for it. 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-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
