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

[Sam Panzer]

Fixed, and finally committed as r162248.


On Mon, Aug 20, 2012 at 4:37 PM, Richard Smith <richard at metafoo.co.uk>wrote:

> Couple of tiny things, then LGTM:
>
> +    *CallExpr = FinishOverloadedCallExpr(*this, S, Fn, Fn, Loc, &Range, 1,
> +                                         Loc, 0, CandidateSet, &Best,
> +                                         OverloadResult,
> +                                         /*AllowTypoCorrection=*/false);
> +    if (CallExpr->isInvalid() || OverloadResult == OR_Deleted) {
>
> The second condition would be clearer as OverloadResult != OR_Success.
>
> +      ForRangeStatus RangeStatus =
> +          BuildNonArrayForRange(*this, S, BeginRangeRef.get(),
> +                                EndRangeRef.get(), RangeType,
> +                                BeginVar, EndVar, ColonLoc, &CandidateSet,
> +                                &BeginExpr, &EndExpr, &BEFFailure);
> +
> +      // If building the range failed, try dereferencing the range
> expression
> +      // unless a diagnostic was issued or the end function is
> problematic.
> +      if (RangeStatus != FRS_Success) {
>
> This 'if' is now redundant; all the code within it is conditioned within
> 'if (RangeStatus == FRS_NoViableFunction)'. I suggest you change this to
> check for FRS_NoViableFunction and remove the newly-redundant inner checks.
>
> On Mon, Aug 20, 2012 at 3:43 PM, Sam Panzer <panzer at google.com> wrote:
>
>> New patch is ~10% smaller!
>>
>>
>> On Thu, Aug 16, 2012 at 3:14 PM, Richard Smith <richard at metafoo.co.uk>wrote:
>>
>>> 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.
>>>
>>>
>>
>> This wasn't possible before introducing SFINAETrap, though it simplifies
>> the code now as you suggest. After a little adjusting, I moved
>> NoteForRangeBeginEndFunction and FinishForRangeVarDecl back out of Sema.
>>
>>
>>
>>>  > +    }
>>>>> > +  } 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/20120820/3cae9bfd/attachment.html>