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

[Aaron Puchert via Phabricator via cfe-commits]

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 reference.
     SemaRef.Diag(VD->getLocation(), diag::warn_for_range_const_reference_copy)
         << VD << VariableType << E->getType();
     QualType NonReferenceType = VariableType.getNonReferenceType();
----------------
The message here says: "loop variable A has type B but is initialized with type C resulting in a copy".

That's not necessarily true, we just know (I think) that a constructor is called. The constructor might copy, but it might also not. However, the constructed object is bound to a reference, which is potentially misleading. One writes `const X& x : range` and `x` doesn't bind to the return value of `*begin(range)`, but to a temporary constructed from that.

Perhaps a more accurate message would be: "loop variable A of type B binds to a temporary constructed from type C" with the appropriate types.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:2772-2773
     // semantic of the code.
     SemaRef.Diag(VD->getLocation(), diag::warn_for_range_variable_always_copy)
         << VD << RangeInitType;
     QualType NonReferenceType = VariableType.getNonReferenceType();
----------------
The message here is: "loop variable A is always a **copy** because the range of type B does not return a reference". This is somewhat misleading, some ranges create objects on-the-fly, in which case these aren't copies. (Assuming RVO the copy constructor might have never been called.)

I would suggest to rephrase this as something like "loop variable A binds to a temporary value produced by a range of type B".


================
Comment at: clang/lib/Sema/SemaStmt.cpp:2823-2824
   // if doing so will prevent a copy.
   SemaRef.Diag(VD->getLocation(), diag::warn_for_range_copy)
       << VD << VariableType << InitExpr->getType();
   SemaRef.Diag(VD->getBeginLoc(), diag::note_use_reference_type)
----------------
Given that `VariableType == VD->getType()` and  `InitExpr->getType() == VD->getInit()->getType()`, can these two types ever be different other than cv-qualifiers? We only call this function if `VariableType` is not a reference type.

Which begs the question: which value provides C in "loop variable A of type B creates a copy from type C"? I would suggest to omit the second type and write: "loop variable A creates a copy of type B", perhaps with stripping const from B.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73007/new/

https://reviews.llvm.org/D73007