[cfe-commits] [patch] Better diagnostics in for-range expressions.

[Richard Smith]

On Thu, Aug 16, 2012 at 2:56 PM, Sam Panzer <panzer at google.com> wrote:

> Here's the next update for this patch.
>
>
> On Wed, Aug 15, 2012 at 4:29 PM, Richard Smith <richard at metafoo.co.uk>wrote:
>
[...]

> > +  if (CXXRecordDecl *D = RangeType->getAsCXXRecordDecl()) {
>
> > +    // - if _RangeT is a class type, the unqualified-ids begin and end
>> are
>> > +    //   looked up in the scope of class _RangeT as if by class member
>> access
>> > +    //   lookup (3.4.5), and if either (or both) finds at least one
>> > +    //   declaration, begin-expr and end-expr are __range.begin() and
>> > +    //   __range.end(), respectively;
>> > +    SemaRef.LookupQualifiedName(BeginMemberLookup, D);
>> > +    SemaRef.LookupQualifiedName(EndMemberLookup, D);
>> > +
>> > +    if (BeginMemberLookup.empty() != EndMemberLookup.empty()) {
>> > +      *BEF = BeginMemberLookup.empty() ? Sema::BEF_end :
>> Sema::BEF_begin;
>> > +      return Sema::FRS_BeginEndMismatch;
>>
>> Can you just issue the diagnostic directly here? I don't think we should
>> attempt recovery in this case. (My "If there's a viable begin() but lookup
>> for end() fails" comment was intended to apply to this case too.)
>>
>>
> Yes, though this means that I have two "diagnostic emitted" error codes:
> one which needs the type and which of begin/end failed to be noted, and a
> second which needs no extra diagnostics.
>

I think previously I suggested that you should emit note_for_range_type
directly in BuildForRangeBeginEndCall; that still seems like the best way
to address this. I would also suggest that you don't emit the note in the
case where FinishForRangeVarDecl fails, since in that case we've already
emitted a diagnostic explaining what we're doing.


> > +    }
>> > +  } else {
>> > +    // - otherwise, begin-expr and end-expr are begin(__range) and
>> > +    //   end(__range), respectively, where begin and end are looked up
>> with
>> > +    //   argument-dependent lookup (3.4.2). For the purposes of this
>> name
>> > +    //   lookup, namespace std is an associated namespace.
>> > +
>> > +  }
>> > +
>>  > +  *BEF = Sema::BEF_begin;
>> > +  Sema::ForRangeStatus RangeStatus =
>> > +      SemaRef.BuildForRangeBeginEndCall(S, ColonLoc, ColonLoc,
>> BeginVar,
>> > +                                        Sema::BEF_begin, BeginNameInfo,
>> > +                                        BeginMemberLookup,
>> CandidateSet,
>> > +                                        BeginRange, BeginExpr);
>> > +  if (RangeStatus != Sema::FRS_Success)
>> > +    return RangeStatus;
>> > +
>> > +  *BEF = Sema::BEF_end;
>> > +  RangeStatus =
>> > +      SemaRef.BuildForRangeBeginEndCall(S, ColonLoc, ColonLoc, EndVar,
>> > +                                        Sema::BEF_end, EndNameInfo,
>> > +                                        EndMemberLookup, CandidateSet,
>> > +                                        EndRange, EndExpr);
>> > +  return RangeStatus;
>> > +}
>> > +
>> > +/// Speculatively attempt to dereference an invalid range expression.
>> > +/// This function will not emit diagnostics, but returns StmtError if
>> > +/// an error occurs.
>> > +static StmtResult RetryWithDereferencedRange(Sema &SemaRef, Scope *S,
>>
>> Please include something about ForRange in this function name.
>>
>
> How about RebuildForRangeWithDereference?
>

SGTM
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120816/6daaaa2d/attachment.html>