Fixed, and finally committed as r162248.
On Mon, Aug 20, 2012 at 4:37 PM, Richard Smith <[email protected]>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 <[email protected]> wrote: > >> New patch is ~10% smaller! >> >> >> On Thu, Aug 16, 2012 at 3:14 PM, Richard Smith <[email protected]>wrote: >> >>> On Thu, Aug 16, 2012 at 2:56 PM, Sam Panzer <[email protected]> wrote: >>> >>>> Here's the next update for this patch. >>>> >>>> >>>> On Wed, Aug 15, 2012 at 4:29 PM, Richard Smith >>>> <[email protected]>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 >>> >> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
