Fixed, and finally committed as r162248.<div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Aug 20, 2012 at 4:37 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>Couple of tiny things, then LGTM:</div><div><br></div><div>+ *CallExpr = FinishOverloadedCallExpr(*this, S, Fn, Fn, Loc, &Range, 1,</div>
<div>+ Loc, 0, CandidateSet, &Best,</div>
<div>+ OverloadResult,</div><div>+ /*AllowTypoCorrection=*/false);</div><div>+ if (CallExpr->isInvalid() || OverloadResult == OR_Deleted) {</div>
<div><br></div><div>The second condition would be clearer as OverloadResult != OR_Success.</div><div><br></div><div><div>+ ForRangeStatus RangeStatus =</div><div>+ BuildNonArrayForRange(*this, S, BeginRangeRef.get(),</div>
<div>+ EndRangeRef.get(), RangeType,</div><div class="im"><div>+ BeginVar, EndVar, ColonLoc, &CandidateSet,</div><div>+ &BeginExpr, &EndExpr, &BEFFailure);</div>
<div>+</div></div><div>+ // If building the range failed, try dereferencing the range expression</div><div>+ // unless a diagnostic was issued or the end function is problematic.</div><div class="im"><div>+ if (RangeStatus != FRS_Success) {</div>
</div></div><div><br></div><div>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.</div>
<div class="HOEnZb"><div class="h5">
<div><br></div><div class="gmail_quote">On Mon, Aug 20, 2012 at 3:43 PM, Sam Panzer <span dir="ltr"><<a href="mailto:panzer@google.com" target="_blank">panzer@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
New patch is ~10% smaller!<br><div class="gmail_extra"><br><br><div class="gmail_quote"><div>On Thu, Aug 16, 2012 at 3:14 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div>On Thu, Aug 16, 2012 at 2:56 PM, Sam Panzer <span dir="ltr"><<a href="mailto:panzer@google.com" target="_blank">panzer@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Here's the next update for this patch.<br><div class="gmail_extra"><br><br><div class="gmail_quote"><div>On Wed, Aug 15, 2012 at 4:29 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
</div></div></div></blockquote></div><div>[...] </div><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"><div class="gmail_quote">
<div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> + if (CXXRecordDecl *D = RangeType->getAsCXXRecordDecl()) {</blockquote></div><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div>> + // - if _RangeT is a class type, the unqualified-ids begin and end are</div>
<div>> + // looked up in the scope of class _RangeT as if by class member access</div><div>> + // lookup (3.4.5), and if either (or both) finds at least one</div><div>> + // declaration, begin-expr and end-expr are __range.begin() and</div>
<div>> + // __range.end(), respectively;</div><div>> + SemaRef.LookupQualifiedName(BeginMemberLookup, D);</div><div>> + SemaRef.LookupQualifiedName(EndMemberLookup, D);</div><div>> +</div><div>> + if (BeginMemberLookup.empty() != EndMemberLookup.empty()) {</div>
</div><div><div>> + *BEF = BeginMemberLookup.empty() ? Sema::BEF_end : Sema::BEF_begin;</div><div>> + return Sema::FRS_BeginEndMismatch;</div><div><br></div></div><div>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.)</div>
<div>
<div><br></div></div></blockquote><div><br></div></div><div>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.</div>
</div></div></blockquote><div><br></div></div><div>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.</div>
<div><div>
<div> </div></div></div></div></blockquote><div><br></div></div><div>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.</div>
<div><div>
<div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_extra"><div class="gmail_quote"><div><div><div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div></div><div>> + }</div><div>> + } else {</div><div>> + // - otherwise, begin-expr and end-expr are begin(__range) and</div>
<div>> + // end(__range), respectively, where begin and end are looked up with</div>
<div>> + // argument-dependent lookup (3.4.2). For the purposes of this name</div><div>> + // lookup, namespace std is an associated namespace.</div><div>> +</div><div>> + }</div><div>> +</div></div>
<div><div>
> + *BEF = Sema::BEF_begin;</div><div>> + Sema::ForRangeStatus RangeStatus =</div><div>> + SemaRef.BuildForRangeBeginEndCall(S, ColonLoc, ColonLoc, BeginVar,</div><div>> + Sema::BEF_begin, BeginNameInfo,</div>
<div>> + BeginMemberLookup, CandidateSet,</div></div><div>> + BeginRange, BeginExpr);</div><div><div>> + if (RangeStatus != Sema::FRS_Success)</div>
<div>> + return RangeStatus;</div><div>> +</div><div>> + *BEF = Sema::BEF_end;</div><div>> + RangeStatus =</div><div>> + SemaRef.BuildForRangeBeginEndCall(S, ColonLoc, ColonLoc, EndVar,</div><div>
> + Sema::BEF_end, EndNameInfo,</div><div>> + EndMemberLookup, CandidateSet,</div></div><div><div>> + EndRange, EndExpr);</div>
<div>> + return RangeStatus;</div><div>> +}</div><div>> +</div></div><div>> +/// Speculatively attempt to dereference an invalid range expression.</div><div>> +/// This function will not emit diagnostics, but returns StmtError if</div>
<div>> +/// an error occurs.</div><div>> +static StmtResult RetryWithDereferencedRange(Sema &SemaRef, Scope *S,</div><div><br></div><div>Please include something about ForRange in this function name.</div></blockquote>
<div><br></div></div></div><div>How about RebuildForRangeWithDereference?</div></div></div></blockquote><div><br></div></div></div><div>SGTM</div></div>
</blockquote></div></div></div><br></div>
</blockquote></div><br>
</div></div></blockquote></div><br></div>