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

[Richard Smith]

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/287efdd3/attachment.html>