Ping<div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Jul 27, 2012 at 3:06 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 is the next version of the patch. The major change consists of reworking the way that I attempt to dereference the range - now it's actually the range that gets a green star.<div><br></div><div>This patch correctly suppresses diagnostics on incorrect attempts to dereference in most cases, but there are extra error messages issued in one instance and a missing clarification in another.</div>
<div>Specifically, if a pointer to a type with a deleted begin() or end() function is passed as the range, there will be a (somewhat mysterious) error complaining about the use of a deleted function in addition to the error explaining that the range was invalid; a similar extra diagnostic appears if begin() or end() is inaccessible (i.e. private) on the pointed-to type.</div>
<div><br></div><div>The missing clarification comes from non-pointer-type ranges which have inaccessible begin() or end() functions. We do not issue a diagnostic that specifies that the range was invalid, because the checks that detect incorrect access occur in the destructor of LookupResult. There is currently no way to tell if the expression was invalid, and we happily continue building the AST as if there were no error.</div>
<div>I included a few test cases that note this behavior and have appropriate FIXME's for when it is corrected.<div class="gmail_extra"><br><div class="gmail_quote"><div class="im">On Thu, Jul 26, 2012 at 2:01 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>Hi Sam,</div><div><br></div><div>Sorry for the massive delay reviewing this...</div><div><br></div><div>> diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td</div>
<div>
> index 52641b8..97b0469 100644</div><div>> --- a/include/clang/Basic/DiagnosticSemaKinds.td</div><div>> +++ b/include/clang/Basic/DiagnosticSemaKinds.td</div><div>> @@ -1391,7 +1391,14 @@ def err_for_range_member_begin_end_mismatch : Error<</div>
<div>> "range type %0 has '%select{begin|end}1' member but no '%select{end|begin}1' member">;</div><div>> def err_for_range_begin_end_types_differ : Error<</div><div>> "'begin' and 'end' must return the same type (got %0 and %1)">;</div>
<div>> +def err_for_range_invalid: Error<</div><div>> + "invalid range expression of type %0">;</div><div>> +def note_for_range_adl_failure: Note<</div><div>> + "no viable '%select{begin|end}0' function for range of type %1">;</div>
<div><br></div><div>Please merge these into a single diagnostic (use the text of note_for_range_adl_failure as the error message).</div></blockquote><div><br></div></div><div>Done.</div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br></div><div>> def note_for_range_type : Note<"range has type %0">;</div>
<div>> +def err_for_range_dereference : Error<</div><div>> + "invalid range expression of type %0; did you mean to dereference it "</div><div>> + "with '*'?">;</div><div>> def note_for_range_begin_end : Note<</div>
<div>> "selected '%select{begin|end}0' %select{function|template }1%2 with iterator type %3">;</div><div>> </div><div>> diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h</div>
<div>> index a48cde0..29fbded 100644</div><div>> --- a/include/clang/Sema/Sema.h</div><div>> +++ b/include/clang/Sema/Sema.h</div><div>> @@ -1923,7 +1923,24 @@ public:</div><div>> Expr **Args, unsigned NumArgs,</div>
<div>> SourceLocation RParenLoc,</div><div>> Expr *ExecConfig,</div><div>> - bool AllowTypoCorrection=true);</div>
<div>> + OverloadCandidateSet *CandidateSet,</div><div>> + bool AllowTypoCorrection,</div><div>> + bool InRangeExpr);</div>
<div>> +</div><div>> + ExprResult BuildOverloadedCallExpr(Scope *S, Expr *Fn,</div><div>> + UnresolvedLookupExpr *ULE,</div><div>> + SourceLocation LParenLoc,</div>
<div>> + Expr **Args, unsigned NumArgs,</div><div>> + SourceLocation RParenLoc,</div><div>> + Expr *ExecConfig,</div>
<div>> + bool AllowTypoCorrection=true,</div><div>> + bool InRangeExpr=false);</div><div><br></div><div>I don't like passing InRangeExpr in here. It's too specific. It's also unnecessary (more on that later).</div>
</blockquote><div><br></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 class="im"><div>> +// FIXME: Update this stale comment</div>
<div>
<br></div><div>Please do :)</div></div></blockquote><div><br></div><div>Done. The next comment too :)</div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>
<br>
</div><div>> +/// ResolveOverloadedCallFn - Given the call expression that calls Fn</div>
<div>> +/// (which eventually refers to the declaration Func) and the call</div><div>> +/// arguments Args/NumArgs, attempt to resolve the function call down</div><div>> +/// to a specific function. If overload resolution succeeds, returns</div>
<div>> +/// the function declaration produced by overload</div><div>> +/// resolution. Otherwise, emits diagnostics, deletes all of the</div><div>> +/// arguments and Fn, and returns NULL.</div><div>> +</div>
<div>
> +ExprResult Sema::BuildOverloadedCallExpr(Scope *S, Expr *Fn,</div><div>> + UnresolvedLookupExpr *ULE,</div><div>> + SourceLocation LParenLoc,</div>
<div>> + Expr **Args, unsigned NumArgs,</div><div>> + SourceLocation RParenLoc,</div><div>> + Expr *ExecConfig,</div>
<div>> + OverloadCandidateSet *CandidateSet,</div><div>> + bool AllowTypoCorrection,</div><div>> + bool InRangeExpr) {</div>
<div>> + if (CandidateSet->empty())</div><div>> return BuildRecoveryCallExpr(*this, S, Fn, ULE, LParenLoc,</div><div>> llvm::MutableArrayRef<Expr *>(Args, NumArgs),</div>
<div>> RParenLoc, /*EmptyLookup=*/true,</div><div>> AllowTypoCorrection);</div><div>> - }</div><div>> -</div><div>> - UnbridgedCasts.restore();</div>
<div>> </div><div>> OverloadCandidateSet::iterator Best;</div><div>> - switch (CandidateSet.BestViableFunction(*this, Fn->getLocStart(), Best)) {</div><div>> + switch (CandidateSet->BestViableFunction(*this, Fn->getLocStart(), Best)) {</div>
<div>> case OR_Success: {</div><div>> FunctionDecl *FDecl = Best->Function;</div><div>> MarkFunctionReferenced(Fn->getExprLoc(), FDecl);</div><div>> @@ -9770,6 +9773,10 @@ Sema::BuildOverloadedCallExpr(Scope *S, Expr *Fn, UnresolvedLookupExpr *ULE,</div>
<div>> }</div><div>> </div><div>> case OR_No_Viable_Function: {</div><div>> + // In case we're in a range expression, let the range handling code take</div><div>> + // care of it.</div><div>
> + if (InRangeExpr)</div><div>> + break;</div><div><br></div><div>This is the only use of InRangeExpr in this function, and we don't call this function in the case where BestViableFunction returns OR_No_Viable_Function. Is this parameter really needed?</div>
</blockquote><div><br></div></div></div><div>Nope, it's gone now.</div><div><div class="h5"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>> +/// This version of BuildOverloadedCallExpr creates an OverloadCandidateSet</div>
<div>> +/// first.</div><div>> +ExprResult Sema::BuildOverloadedCallExpr(Scope *S, Expr *Fn,</div>
<div>> + UnresolvedLookupExpr *ULE,</div><div>> + SourceLocation LParenLoc,</div><div>> + Expr **Args, unsigned NumArgs,</div>
<div>> + SourceLocation RParenLoc,</div><div>> + Expr *ExecConfig,</div><div>> + bool AllowTypoCorrection,</div>
<div>> + bool InRangeExpr) {</div><div>> +#ifndef NDEBUG</div><div>> + if (ULE->requiresADL()) {</div><div>> + // To do ADL, we must have found an unqualified name.</div>
<div>> + assert(!ULE->getQualifier() && "qualified name with ADL");</div><div>> +</div><div>> + // We don't perform ADL for implicit declarations of builtins.</div><div>> + // Verify that this was correctly set up.</div>
<div>> + FunctionDecl *F;</div><div>> + if (ULE->decls_begin() + 1 == ULE->decls_end() &&</div><div>> + (F = dyn_cast<FunctionDecl>(*ULE->decls_begin())) &&</div><div>
> + F->getBuiltinID() && F->isImplicit())</div>
<div>> + llvm_unreachable("performing ADL for builtin");</div><div>> +</div><div>> + // We don't perform ADL in C.</div><div>> + assert(getLangOpts().CPlusPlus && "ADL enabled in C");</div>
<div>> + } else</div><div>> + assert(!ULE->isStdAssociatedNamespace() &&</div><div>> + "std is associated namespace but not doing ADL");</div><div>> +#endif</div><div><br></div>
<div>This checking belongs in buildOverloadedCallSet. We want to perform these checks for range-based for calls too.</div></blockquote><div><br></div></div></div><div>Done.</div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br></div><div>> + </div><div>> + OverloadCandidateSet CandidateSet(Fn->getExprLoc());</div>
<div>> + ExprResult result;</div><div>> + if (buildOverloadedCallSet(S, Fn, ULE, Args, NumArgs, LParenLoc,</div><div>> + &CandidateSet, &result))</div><div>> + return result;</div>
<div>> +</div><div>> + return BuildOverloadedCallExpr(S, Fn, ULE, LParenLoc, Args, NumArgs,</div><div>> + RParenLoc, ExecConfig, &CandidateSet,</div><div>> + AllowTypoCorrection, InRangeExpr);</div>
<div>> +</div><div>> }</div><div>> </div><div>> static bool IsOverloaded(const UnresolvedSetImpl &Functions) {</div><div>> diff --git a/lib/Sema/SemaStmt.cpp b/lib/Sema/SemaStmt.cpp</div><div>> index 9be1d34..18bcf89 100644</div>
<div>> --- a/lib/Sema/SemaStmt.cpp</div><div>> +++ b/lib/Sema/SemaStmt.cpp</div><div>> @@ -1578,18 +1578,35 @@ void NoteForRangeBeginEndFunction(Sema &SemaRef, Expr *E,</div><div>> << BEF << IsTemplate << Description << E->getType();</div>
<div>> }</div><div>> </div><div>> +</div><div>> +enum ForRangeStatus {</div><div>> + FRS_Success,</div><div>> + FRS_InvalidMemberFunction,</div><div>> + FRS_NoOverload,</div><div>> + FRS_DeletedOrAmbiguous,</div>
<div>> + FRS_NoViableFunction,</div><div>> + FRS_InvalidIteratorType,</div><div>> + FRS_BeginEndMismatch</div><div>> +};</div><div>> +</div><div>> /// Build a call to 'begin' or 'end' for a C++0x for-range statement. If the</div>
<div>> /// given LookupResult is non-empty, it is assumed to describe a member which</div><div>> /// will be invoked. Otherwise, the function will be found via argument</div><div>> /// dependent lookup.</div>
<div>
> -static ExprResult BuildForRangeBeginEndCall(Sema &SemaRef, Scope *S,</div><div>> - SourceLocation Loc,</div><div>> - VarDecl *Decl,</div>
<div>> - BeginEndFunction BEF,</div><div>> - const DeclarationNameInfo &NameInfo,</div><div>> - LookupResult &MemberLookup,</div>
<div>> - Expr *Range) {</div><div>> - ExprResult CallExpr;</div><div>> +/// CallExpr is set and FRS_Success returned on success, otherwise some</div><div>> +/// non-success value is returned.</div>
<div>> +static ForRangeStatus</div><div>> +BuildForRangeBeginEndCall(Sema &SemaRef, Scope *S,</div><div>> + SourceLocation Loc,</div><div>> + SourceLocation RangeLoc,</div>
<div>> + VarDecl *Decl,</div><div>> + BeginEndFunction BEF,</div><div>> + const DeclarationNameInfo &NameInfo,</div><div>> + LookupResult &MemberLookup,</div>
<div>> + OverloadCandidateSet *CandidateSet,</div><div>> + Expr *Range,</div><div>> + ExprResult *CallExpr) {</div><div><br></div><div>Now this is performing the call to BestViableFunction itself, this really belongs in SemaOverload.cpp.</div>
<div><br></div></blockquote><div><br></div></div></div><div>Done. It took some rearranging :)</div><div>Side-effects of the move include shifting the ForRangeStatus and BeginEndFunction enums into Sema, along with FinishForRangeVarDecl(). If this isn't as nice of a result as you had hoped, I can work on moving it back.</div>
<div><div class="h5">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div></div><div>> + CandidateSet->clear();</div><div>> if (!MemberLookup.empty()) {</div><div>
> ExprResult MemberRef =</div><div>> SemaRef.BuildMemberReferenceExpr(Range, Range->getType(), Loc,</div>
<div>> @@ -1598,12 +1615,16 @@ static ExprResult BuildForRangeBeginEndCall(Sema &SemaRef, Scope *S,</div><div>> /*FirstQualifierInScope=*/0,</div><div>> MemberLookup,</div>
<div>> /*TemplateArgs=*/0);</div><div>> - if (MemberRef.isInvalid())</div><div>> - return ExprError();</div><div>> - CallExpr = SemaRef.ActOnCallExpr(S, MemberRef.get(), Loc, MultiExprArg(),</div>
<div>> + if (MemberRef.isInvalid()) {</div><div>> + *CallExpr = ExprError();</div><div>> + return FRS_InvalidMemberFunction;</div><div>> + }</div><div>> + *CallExpr = SemaRef.ActOnCallExpr(S, MemberRef.get(), Loc, MultiExprArg(),</div>
<div>> Loc, 0);</div><div>> - if (CallExpr.isInvalid())</div><div>> - return ExprError();</div><div>> + if (CallExpr->isInvalid()) {</div><div>> + *CallExpr = ExprError();</div>
<div>> + return FRS_InvalidMemberFunction;</div><div>> + }</div><div>> } else {</div><div>> UnresolvedSet<0> FoundNames;</div><div>> // C++0x [stmt.ranged]p1: For the purposes of this name lookup, namespace</div>
<div>> @@ -1614,20 +1635,49 @@ static ExprResult BuildForRangeBeginEndCall(Sema &SemaRef, Scope *S,</div><div>> /*NeedsADL=*/true, /*Overloaded=*/false,</div><div>> FoundNames.begin(), FoundNames.end(),</div>
<div>> /*LookInStdNamespace=*/true);</div><div>> - CallExpr = SemaRef.BuildOverloadedCallExpr(S, Fn, Fn, Loc, &Range, 1, Loc,</div><div>> - 0, /*AllowTypoCorrection=*/false);</div>
<div>> - if (CallExpr.isInvalid()) {</div><div>> - SemaRef.Diag(Range->getLocStart(), diag::note_for_range_type)</div><div>> - << Range->getType();</div><div>> - return ExprError();</div>
<div>> +</div><div>> + SemaRef.buildOverloadedCallSet(S, Fn, Fn, &Range, 1, Loc, CandidateSet,</div><div>> + CallExpr);</div><div>> +</div><div>> + OverloadCandidateSet::iterator Best;</div>
<div>> + if (CandidateSet->empty()) {</div><div>> + *CallExpr = ExprError();</div><div>> + return FRS_NoOverload;</div><div>> + } else {</div><div>> + switch (CandidateSet->BestViableFunction(SemaRef, Fn->getLocStart(),</div>
<div>> + Best)) {</div><div>> + case OR_Success:</div><div>> + *CallExpr =</div><div>> + SemaRef.BuildOverloadedCallExpr(S, Fn, Fn, Loc, &Range, 1, Loc, 0,</div>
<div>> + CandidateSet,</div><div>> + /*AllowTypoCorrection=*/false,</div><div>> + /*InRangeExpr=*/true);</div>
<div><br></div><div>In the common case of a success, we're calling BestViableFunction twice; once here and once in BuildOverloadedCallExpr. Can you factor out the 'OR_Success' case from there and just call it directly?</div>
</blockquote><div><br></div></div></div><div>Done.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br></div><div>> + if (CallExpr->isInvalid())</div><div>> + return FRS_NoOverload;</div><div>> + break;</div><div>> +</div><div>> + case OR_No_Viable_Function:</div><div>
> + *CallExpr = ExprError();</div><div>> + return FRS_NoViableFunction;</div><div>> +</div><div>> + case OR_Deleted:</div><div>> + case OR_Ambiguous:</div><div>> + // Here we want to defer to the diagnostics in BuildOverloadedCallExpr.</div>
<div>> + // The return value doesn't matter, since we're returning an ExprError()</div><div>> + // anyway.</div><div>> + SemaRef.BuildOverloadedCallExpr(S, Fn, Fn, Loc, &Range, 1, Loc, 0,</div>
<div>> + /*AllowTypoCorrection=*/false,</div><div>> + /*InRangeExpr=*/true);</div><div>> + *CallExpr = ExprError();</div><div>
> + return FRS_DeletedOrAmbiguous;</div>
<div><br></div><div>This case is a bit awkward: we're calling BuildOverloadedCallExpr in order to generate diagnostics which we don't actually want (the diagnostics we do want are produced later by ForRangeDiagnostics). How about producing the diagnostics directly here, instead of calling BuildOverloadedCallExpr?</div>
</blockquote><div><br></div></div><div>I was looking to avoid duplicating the code that emits the diagnostic, but the cure was worse. Done.</div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br></div><div>> + }</div><div>> }</div><div>> +</div><div>> }</div><div>> - if (FinishForRangeVarDecl(SemaRef, Decl, CallExpr.get(), Loc,</div><div>> - diag::err_for_range_iter_deduction_failure)) {</div>
<div>> - NoteForRangeBeginEndFunction(SemaRef, CallExpr.get(), BEF);</div><div>> - return ExprError();</div><div>> - }</div><div>> - return CallExpr;</div><div>> + if (FinishForRangeVarDecl(SemaRef, Decl, CallExpr->get(), Loc,</div>
<div>> + diag::err_for_range_iter_deduction_failure))</div><div>> + return FRS_InvalidIteratorType;</div><div>> + return FRS_Success;</div><div>> }</div><div>> </div><div>> }</div>
<div>> @@ -1700,6 +1750,129 @@ Sema::ActOnCXXForRangeStmt(SourceLocation ForLoc, SourceLocation LParenLoc,</div><div>> RParenLoc);</div><div>> }</div><div>> </div><div>> +// Emit the diagnostic appropriate to the given ForRangeStatus. The other</div>
<div>> +// parameters are used for supplementary information.</div><div>> +static void ForRangeDiagnostics(Sema &SemaRef, ForRangeStatus st, Expr *Range,</div><div>> + const ExprResult &CallExpr,</div>
<div>> + SourceLocation RangeLoc,</div><div>> + OverloadCandidateSet *CandidateSet,</div><div>> + BeginEndFunction BEF) {</div>
<div>> + switch (st) {</div><div>> + case FRS_Success:</div><div>> + // It worked; nothing to do.</div><div>> + break;</div><div>> +</div><div>> + case FRS_InvalidMemberFunction:</div><div>> + SemaRef.Diag(Range->getLocStart(), diag::err_for_range_invalid)</div>
<div>> + << RangeLoc << Range->getType();</div><div>> + SemaRef.Diag(Range->getLocStart(), diag::note_for_range_adl_failure)</div><div>> + << BEF << Range->getType();</div>
<div>> + break;</div><div>> +</div><div>> + case FRS_NoOverload:</div><div>> + SemaRef.Diag(Range->getLocStart(), diag::err_for_range_invalid)</div><div>> + << RangeLoc << Range->getType();</div>
<div>> + SemaRef.Diag(Range->getLocStart(), diag::note_for_range_adl_failure)</div><div>> + << BEF << Range->getType();</div><div>> + break;</div><div>> +</div><div>> + case FRS_DeletedOrAmbiguous:</div>
<div>> + // Some of the diagnostics are emitted in BuildOverloadedCallExpr, so we</div><div>> + // just add these.</div><div>> + SemaRef.Diag(Range->getLocStart(), diag::err_for_range_invalid)</div><div>
> + << RangeLoc << Range->getType();</div><div>> + SemaRef.Diag(Range->getLocStart(), diag::note_for_range_adl_failure)</div><div>> + << BEF << Range->getType();</div>
<div>> + break;</div><div>> +</div><div>> + case FRS_NoViableFunction:</div><div>> + SemaRef.Diag(Range->getLocStart(), diag::err_for_range_invalid)</div><div>> + << RangeLoc << Range->getType();</div>
<div>> + CandidateSet->NoteCandidates(SemaRef, OCD_AllCandidates,</div><div>> + llvm::makeArrayRef(&Range, /*NumArgs=*/1));</div><div>> + SemaRef.Diag(Range->getLocStart(), diag::note_for_range_adl_failure)</div>
<div>> + << BEF << Range->getType();</div><div>> + break;</div><div>> +</div><div>> + case FRS_InvalidIteratorType:</div><div>> + NoteForRangeBeginEndFunction(SemaRef, CallExpr.get(), BEF);</div>
<div>> + break;</div><div>> +</div><div>> + case FRS_BeginEndMismatch:</div><div>> + SemaRef.Diag(RangeLoc, diag::err_for_range_member_begin_end_mismatch)</div><div>> + << Range->getType() << BEF;</div>
<div>> + break;</div><div>> + }</div><div>> +}</div><div><br></div><div>I don't like that we sometimes produce notes here which attach to diagnostics produced in BuildForRangeBeginEndCall. I'd like to generally see this restructured as follows:</div>
<div> <br></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div></div><div>BuildForRangeBeginEndCall's return value indicates either 'everything was OK', 'this isn't a valid range because there's no viable begin/end function', or 'something else went wrong and I produced a diagnostic'. Then, produce a diagnostic here only in the second case.</div>
</blockquote><div><br></div></div></div><div>Done. This required that I move NoteForRangeBeginEndFunction() into Sema so that BuildForRangeBeginEndCall() could call it.</div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br></div><div>> +</div><div>> +/// \brief Create the initialization, compare, and increment steps for</div><div>> +/// the range-based for loop expression.</div><div>> +/// This function does not handle array-based for loops,</div>
<div>> +/// which are created in Sema::BuildCXXForRangeStmt.</div><div>> +///</div><div>> +/// \returns a ForRangeStatus indicating success or what kind of error occurred.</div><div>> +/// BeginExpr and EndExpr are set and FRS_Success is returned on success;</div>
<div>> +/// CandidateSet and BEF are set and some non-success value is returned on</div><div>> +/// failure.</div><div>> +static ForRangeStatus BuildNonArrayForRange(Sema &SemaRef, Scope *S,</div><div>> + Expr *BeginRange, Expr *EndRange,</div>
<div>> + QualType RangeType,</div><div>> + VarDecl *BeginVar,</div><div>> + VarDecl *EndVar,</div>
<div>> + SourceLocation ColonLoc,</div><div>> + OverloadCandidateSet *CandidateSet,</div><div>> + ExprResult *BeginExpr,</div>
<div>> + ExprResult *EndExpr,</div><div>> + BeginEndFunction *BEF) {</div><div>> + DeclarationNameInfo BeginNameInfo(</div><div>
> + &SemaRef.PP.getIdentifierTable().get("begin"), ColonLoc);</div><div>> + DeclarationNameInfo EndNameInfo(&SemaRef.PP.getIdentifierTable().get("end"),</div><div>> + ColonLoc);</div>
<div>> +</div><div>> + LookupResult BeginMemberLookup(SemaRef, BeginNameInfo,</div><div>> + Sema::LookupMemberName);</div><div>> + LookupResult EndMemberLookup(SemaRef, EndNameInfo, Sema::LookupMemberName);</div>
<div>> +</div><div>> + if (CXXRecordDecl *D = RangeType->getAsCXXRecordDecl()) {</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>> + *BEF = BeginMemberLookup.empty() ? BEF_end : BEF_begin;</div><div>> + return FRS_BeginEndMismatch;</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>> + *BEF = BEF_begin;</div><div>> + ForRangeStatus</div><div>> + RangeStatus = BuildForRangeBeginEndCall(SemaRef, S, ColonLoc,</div><div>> + ColonLoc, BeginVar,</div>
<div>> + BEF_begin, BeginNameInfo,</div><div>> + BeginMemberLookup,</div><div>> + CandidateSet,</div>
<div>> + BeginRange, BeginExpr);</div><div>> + if (RangeStatus != FRS_Success)</div><div>> + return st;</div><div>> +</div><div>> + *BEF = BEF_end;</div><div>
> + RangeStatus = BuildForRangeBeginEndCall(SemaRef, S, ColonLoc, ColonLoc,</div><div>> + EndVar, BEF_end, EndNameInfo,</div><div>> + EndMemberLookup, CandidateSet,</div>
<div>> + EndRange, EndExpr);</div><div>> + return RangeStatus;</div><div>> +}</div><div>> +</div><div>> /// BuildCXXForRangeStmt - Build or instantiate a C++0x for-range statement.</div>
<div>> StmtResult</div><div>> Sema::BuildCXXForRangeStmt(SourceLocation ForLoc, SourceLocation ColonLoc,</div><div>> @@ -1791,49 +1964,58 @@ Sema::BuildCXXForRangeStmt(SourceLocation ForLoc, SourceLocation ColonLoc,</div>
<div>> return StmtError();</div><div>> }</div><div>> } else {</div><div>> - DeclarationNameInfo BeginNameInfo(&PP.getIdentifierTable().get("begin"),</div><div>> - ColonLoc);</div>
<div>> - DeclarationNameInfo EndNameInfo(&PP.getIdentifierTable().get("end"),</div><div>> - ColonLoc);</div><div>> -</div><div>> - LookupResult BeginMemberLookup(*this, BeginNameInfo, LookupMemberName);</div>
<div>> - LookupResult EndMemberLookup(*this, EndNameInfo, LookupMemberName);</div><div>> -</div><div>> - if (CXXRecordDecl *D = RangeType->getAsCXXRecordDecl()) {</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>> - LookupQualifiedName(BeginMemberLookup, D);</div><div>> - LookupQualifiedName(EndMemberLookup, D);</div><div>> -</div><div>> - if (BeginMemberLookup.empty() != EndMemberLookup.empty()) {</div>
<div>> - Diag(ColonLoc, diag::err_for_range_member_begin_end_mismatch)</div><div>> - << RangeType << BeginMemberLookup.empty();</div><div>> + Expr *BeginRange = BeginRangeRef.get();</div>
<div>> + Expr *EndRange = EndRangeRef.get();</div><div>> + OverloadCandidateSet CandidateSet(RangeLoc);</div><div>> + BeginEndFunction BEFFailure;</div><div>> + ForRangeStatus RangeStatus =</div>
<div>> + BuildNonArrayForRange(*this, S, BeginRange, EndRange, RangeType,</div><div>> + BeginVar, EndVar, ColonLoc, &CandidateSet,</div><div>> + &BeginExpr, &EndExpr, &BEFFailure);</div>
<div>> +</div><div>> + // Attempt to dereference pointers and try again.</div><div>> + if (RangeStatus != FRS_Success) {</div><div>> + if (!RangeType->isPointerType()) {</div><div>> + const ExprResult & TheCall = BEFFailure ? EndExpr: BeginExpr;</div>
<div>> + ForRangeDiagnostics(*this, RangeStatus,</div><div>> + BEFFailure ? EndRange : BeginRange,</div><div>> + TheCall, RangeLoc, &CandidateSet, BEFFailure);</div>
<div>> return StmtError();</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>> + BeginRange =</div><div>> + new (Context) UnaryOperator(BeginRange, UO_Deref,</div>
<div>> + RangeType->getPointeeType(),</div><div>> + VK_LValue, OK_Ordinary, RangeLoc);</div><div>> + EndRange =</div><div>> + new (Context) UnaryOperator(EndRange, UO_Deref,</div>
<div>> + RangeType->getPointeeType(),</div><div>> + VK_LValue, OK_Ordinary, RangeLoc);</div><div>> +</div><div>> + BeginEndFunction DerefBEF;</div>
<div>> + ForRangeStatus DerefStatus</div><div>> + = BuildNonArrayForRange(*this, S, BeginRange, EndRange,</div><div>> + BeginRange->getType(),</div><div>> + BeginVar, EndVar,</div>
<div>> + ColonLoc, &CandidateSet,</div><div>> + &BeginExpr, &EndExpr, &DerefBEF);</div><div>> + if (DerefStatus != FRS_Success) {</div>
<div>> + const ExprResult & TheCall = BEFFailure ? EndExpr: BeginExpr;</div><div>> + ForRangeDiagnostics(*this, RangeStatus,</div><div>> + DerefBEF ? EndRangeRef.get(): BeginRangeRef.get(),</div>
<div>> + TheCall, RangeLoc, &CandidateSet, BEFFailure);</div><div>> + return StmtError();</div><div>> + } else {</div><div>> + // The attempt to dereference would likely succeed.</div>
<div>> + Diag(RangeLoc, diag::err_for_range_dereference)</div><div>> + << RangeLoc << RangeType</div><div>> + << FixItHint::CreateInsertion(RangeLoc, "*");</div>
<div><br></div><div>Can you make this comment a bit more precise? The bar for producing a fixit on an error is that the fix would succeed and would produce the same AST. Here, we're taking some liberties, because we're applying the '*' within the __begin and __end expressions instead of to the __range initializer. That's equivalent, but doesn't provide an identical AST, so you should either justify it or rebuild __range with the right expression here.</div>
</blockquote><div><br></div></div></div><div>I changed it to rebuild the correct expression here by introducing mutual recursion with ActOnCXXForRangeStmt, limiting the depth to one.</div><div><div class="h5"><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br></div><div>> + }</div><div>> }</div><div>> -</div><div>> - BeginExpr = BuildForRangeBeginEndCall(*this, S, ColonLoc, BeginVar,</div><div>> - BEF_begin, BeginNameInfo,</div>
<div>> - BeginMemberLookup,</div><div>> - BeginRangeRef.get());</div><div>> - if (BeginExpr.isInvalid())</div><div>> - return StmtError();</div>
<div>> -</div><div>> - EndExpr = BuildForRangeBeginEndCall(*this, S, ColonLoc, EndVar,</div><div>> - BEF_end, EndNameInfo,</div><div>> - EndMemberLookup, EndRangeRef.get());</div>
<div>> - if (EndExpr.isInvalid())</div><div>> - return StmtError();</div><div>> }</div><div>> </div><div>> + assert(!BeginExpr.isInvalid() && !EndExpr.isInvalid() &&</div>
<div>> + "invalid range expression in for loop");</div><div>> +</div><div>> // C++0x [decl.spec.auto]p6: BeginType and EndType must be the same.</div><div>> QualType BeginType = BeginVar->getType(), EndType = EndVar->getType();</div>
<div>> if (!Context.hasSameType(BeginType, EndType)) {</div><div>> diff --git a/test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp b/test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp</div><div>> index 96bb472..e5809f0 100644</div>
<div>> --- a/test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp</div><div>> +++ b/test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp</div><div>> @@ -3,20 +3,21 @@</div><div>> struct pr12960 {</div><div>> int begin;</div>
<div>> void foo(int x) {</div><div>> - for (int& it : x) { // expected-error {{use of undeclared identifier 'begin'}} expected-note {{range has type 'int'}}</div><div>> + for (int& it : x) { //expected-error{{invalid range expression}}\</div>
<div>> + expected-note{{no viable 'begin' function for range of type 'int'}}</div><div>> }</div><div>> }</div><div>> };</div><div>> </div><div>> namespace std {</div><div>
> template<typename T></div><div>> - auto begin(T &&t) -> decltype(t.begin()) { return t.begin(); } // expected-note 4{{ignored: substitution failure}}</div><div>> + auto begin(T &&t) -> decltype(t.begin()) { return t.begin(); } // expected-note 3{{ignored: substitution failure}}</div>
<div>> template<typename T></div><div>> auto end(T &&t) -> decltype(t.end()) { return t.end(); } // expected-note {{candidate template ignored: substitution failure [with T = }}</div><div>> </div>
<div>> template<typename T></div><div>> auto begin(T &&t) -> decltype(t.alt_begin()) { return t.alt_begin(); } // expected-note {{selected 'begin' template [with T = }} \</div><div>
> - expected-note 4{{candidate template ignored: substitution failure [with T = }}</div>
<div>> + expected-note 3{{candidate template ignored: substitution failure [with T = }}</div><div>> template<typename T></div><div>
> auto end(T &&t) -> decltype(t.alt_end()) { return t.alt_end(); } // expected-note {{candidate template ignored: substitution failure [with T = }}</div><div>> </div><div>> @@ -116,9 +117,11 @@ void g() {</div>
<div>> struct NoEndADL {</div><div>> null_t alt_begin();</div><div>> };</div><div>> - for (auto u : NoBeginADL()) { // expected-error {{no matching function for call to 'begin'}} expected-note {{range has type 'NoBeginADL'}}</div>
<div>> + for (auto u : NoBeginADL()) {// expected-error{{invalid range expression of type 'NoBeginADL'}} \</div><div>> + expected-note{{no viable 'begin' function for range of type 'NoBeginADL'}}</div>
<div>> }</div><div>> - for (auto u : NoEndADL()) { // expected-error {{no matching function for call to 'end'}} expected-note {{range has type 'NoEndADL'}}</div><div>> + for (auto u : NoEndADL()) { // expected-error{{invalid range expression of type 'NoEndADL'}} \</div>
<div>> + expected-note{{no viable 'end' function for range of type 'NoEndADL'}}</div><div>> }</div><div>> </div><div>> struct NoBegin {</div><div>> @@ -156,8 +159,8 @@ void g() {</div>
<div>> for (int n : NoCopy()) { // ok</div><div>> }</div><div>> </div><div>> - for (int n : 42) { // expected-error {{no matching function for call to 'begin'}} \</div><div>> - expected-note {{range has type 'int'}}</div>
<div>> + for (int n : 42) { // expected-error{{invalid range expression of type 'int'}} \</div><div>> + expected-note{{no viable 'begin' function for range of type 'int'}}</div><div>> }</div>
<div>> </div><div>> for (auto a : *also_incomplete) { // expected-error {{cannot use incomplete type 'struct Incomplete' as a range}}</div><div>> @@ -179,9 +182,10 @@ template void h<A(&)[13], int>(A(&)[13]); // expected-note {{requested here}}</div>
<div>> </div><div>> template<typename T></div><div>> void i(T t) {</div><div>> - for (auto u : t) { // expected-error {{no matching function for call to 'begin'}} \</div><div>> - expected-error {{member function 'begin' not viable}} \</div>
<div>> - expected-note {{range has type}}</div><div>> + for (auto u : t) { // expected-error {{invalid range expression of type 'A *'; did you mean to dereference it with '*'?}} \</div>
<div>> + expected-error {{member function 'begin' not viable}}\</div><div>> + expected-error{{invalid range expression of type 'const A'}}\</div><div>> + expected-note{{no viable 'begin' function for range of type 'const A'}}</div>
<div>> }</div><div>> }</div><div>> template void i<A[13]>(A*); // expected-note {{requested here}}</div><div>> @@ -204,7 +208,8 @@ void end(VoidBeginADL);</div><div>> void j() {</div><div>> for (auto u : NS::ADL()) {</div>
<div>> }</div><div>> - for (auto u : NS::NoADL()) { // expected-error {{no matching function for call to 'begin'}} expected-note {{range has type}}</div><div>> + for (auto u : NS::NoADL()) { // expected-error{{invalid range expression of type 'NS::NoADL'}}\</div>
<div>> + expected-note {{no viable 'begin' function for range of type 'NS::NoADL'}}</div><div>> }</div><div>> for (auto a : VoidBeginADL()) { // expected-error {{cannot use type 'void' as an iterator}}</div>
<div>> }</div><div>> @@ -215,4 +220,3 @@ void example() {</div><div>> for (int &x : array)</div><div>> x *= 2;</div><div>> }</div><div>> -</div><div>> diff --git a/test/SemaCXX/for-range-no-std.cpp b/test/SemaCXX/for-range-no-std.cpp</div>
<div>> index fa42ca4..8f3213b 100644</div><div>> --- a/test/SemaCXX/for-range-no-std.cpp</div><div>> +++ b/test/SemaCXX/for-range-no-std.cpp</div><div>> @@ -31,10 +31,11 @@ NS::iter end(NS::NoADL);</div><div>
> void f() {</div>
<div>> int a[] = {1, 2, 3};</div><div>> for (auto b : S()) {} // ok</div><div>> - for (auto b : T()) {} // expected-error {{no matching function for call to 'begin'}} expected-note {{range has type}}</div>
<div>> + for (auto b : T()) {} // expected-error {{invalid range expression of type 'T'}} expected-note {{no viable 'begin' function for range of type 'T'}}</div><div>> for (auto b : a) {} // ok</div>
<div>> for (int b : NS::ADL()) {} // ok</div><div>> - for (int b : NS::NoADL()) {} // expected-error {{no matching function for call to 'begin'}} expected-note {{range has type}}</div><div>> + for (int b : NS::NoADL()) {} // expected-error {{invalid range expression of type 'NS::NoADL'}} \</div>
<div>> + expected-note {{no viable 'begin' function for range of type 'NS::NoADL'}}</div><div>> }</div><div>> </div><div>> void PR11601() {</div><div>> diff --git a/test/SemaCXX/typo-correction.cpp b/test/SemaCXX/typo-correction.cpp</div>
<div>> index 893f08a..f8bb8d0 100644</div><div>> --- a/test/SemaCXX/typo-correction.cpp</div><div>> +++ b/test/SemaCXX/typo-correction.cpp</div><div>> @@ -155,7 +155,8 @@ void Test3() {</div><div>> struct R {};</div>
<div>> bool begun(R);</div><div>> void RangeTest() {</div><div>> - for (auto b : R()) {} // expected-error {{use of undeclared identifier 'begin'}} expected-note {{range has type}}</div><div>> + for (auto b : R()) {} // expected-error {{invalid range expression of type 'R'}}\</div>
<div>> + expected-note {{no viable 'begin' function for range of type 'R'}}</div><div>> }</div><div>> </div><div>> // PR 12019 - Avoid infinite mutual recursion in DiagnoseInvalidRedeclaration</div>
<span><font color="#888888">
<div>> -- </div><div>> 1.7.7.3</div></font></span><div><div><div>> </div><br><div class="gmail_quote">On Mon, Jul 23, 2012 at 3:41 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">Ping.<div><div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jul 11, 2012 at 4:29 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">
Ping, and a slightly updated version which changed a few comments to make doxygen happy and renamed a few variables to fit the conventions better.<div><div><div class="gmail_extra"><br><br><div class="gmail_quote">
On Mon, Jun 18, 2012 at 9:40 AM, 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"><div style="font-family:arial,helvetica,sans-serif"><font>*sigh*<div>I ran format-patch from the correct branch this time.</div>
<div><div><div><div><br><div class="gmail_quote">On Sun, Jun 17, 2012 at 4:49 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>On Thu, Jun 14, 2012 at 10:32 AM, Sam Panzer <<a href="mailto:panzer@google.com" target="_blank">panzer@google.com</a>> wrote:<br>
> I will now be including the word 'attach' in these emails as Gmail's<br>
> equivalent of -Wmissing-patch.<br>
<br>
</div>Sadly that won't save you from running 'diff' in the wrong checkout.<br>
This is the printf / c_str patch.<br>
</blockquote></div><br></div></div></div></div></font></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br>
</div></div></blockquote></div></div></div><br></div>
</div>
</blockquote></div><br></div>