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

Tue Jan 21 14:14:47 PST 2020

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