[cfe-commits] [patch] Better diagnostics in for-range expressions.
Richard Smith
richard at metafoo.co.uk
Wed Aug 15 16:29:27 PDT 2012
A few small things:
> diff --git a/lib/Sema/SemaOverload.cpp b/lib/Sema/SemaOverload.cpp
> index a874489..1323978 100644
> --- a/lib/Sema/SemaOverload.cpp
> +++ b/lib/Sema/SemaOverload.cpp
> @@ -11199,6 +11241,103 @@ ExprResult
Sema::BuildLiteralOperatorCall(LookupResult &R,
> return MaybeBindToTemporary(UDL);
> }
>
> +/// Build a call to 'begin' or 'end' for a C++0x for-range statement. If
the
You may as well change the 'C++0x' references to 'C++11' in all the changed
lines. (Check the quoted paragraphs match the final C++11 standard first,
though.)
> +/// given LookupResult is non-empty, it is assumed to describe a member
which
> +/// will be invoked. Otherwise, the function will be found via argument
> +/// dependent lookup.
> +/// CallExpr is set to a valid expression and FRS_Success returned on
success,
> +/// otherwise CallExpr is set to ExprError() and some non-success value
> +/// is returned.
> +Sema::ForRangeStatus
> +Sema::BuildForRangeBeginEndCall(Scope *S, SourceLocation Loc,
> + SourceLocation RangeLoc, VarDecl *Decl,
> + BeginEndFunction BEF,
> + const DeclarationNameInfo &NameInfo,
> + LookupResult &MemberLookup,
> + OverloadCandidateSet *CandidateSet,
> + Expr *Range, ExprResult *CallExpr) {
> + CandidateSet->clear();
> + if (!MemberLookup.empty()) {
> + ExprResult MemberRef =
> + BuildMemberReferenceExpr(Range, Range->getType(), Loc,
> + /*IsPtr=*/false, CXXScopeSpec(),
> + /*TemplateKWLoc=*/SourceLocation(),
> + /*FirstQualifierInScope=*/0,
> + MemberLookup,
> + /*TemplateArgs=*/0);
> + if (MemberRef.isInvalid()) {
> + *CallExpr = ExprError();
> + return FRS_DiagnosticIssued;
> + }
> + *CallExpr = ActOnCallExpr(S, MemberRef.get(), Loc, MultiExprArg(),
Loc, 0);
> + if (CallExpr->isInvalid()) {
> + *CallExpr = ExprError();
> + return FRS_DiagnosticIssued;
> + }
> + } else {
> + UnresolvedSet<0> FoundNames;
> + // C++0x [stmt.ranged]p1: For the purposes of this name lookup,
namespace
> + // std is an associated namespace.
> + UnresolvedLookupExpr *Fn =
> + UnresolvedLookupExpr::Create(Context, /*NamingClass=*/0,
> + NestedNameSpecifierLoc(), NameInfo,
> + /*NeedsADL=*/true,
/*Overloaded=*/false,
> + FoundNames.begin(), FoundNames.end(),
> + /*LookInStdNamespace=*/true);
> +
> + bool CandidateSetError = buildOverloadedCallSet(S, Fn, Fn, &Range,
1, Loc,
> + CandidateSet,
CallExpr);
> + if (CandidateSet->empty() || CandidateSetError) {
> + *CallExpr = ExprError();
> + return FRS_NoViableFunction;
> + }
> + OverloadCandidateSet::iterator Best;
> + OverloadingResult OverloadResult =
> + CandidateSet->BestViableFunction(*this, Fn->getLocStart(), Best);
> +
> + switch (OverloadResult) {
> + case OR_Success:
> + *CallExpr =
> + FinishOverloadedCallExpr(*this, S, Fn, Fn, Loc, &Range, 1,
Loc, 0,
> + CandidateSet, &Best, OverloadResult,
> + /*AllowTypoCorrection=*/false);
> + if (CallExpr->isInvalid())
> + return FRS_DiagnosticIssued;
> + break;
> +
> + case OR_No_Viable_Function:
> + *CallExpr = ExprError();
> + return FRS_NoViableFunction;
> +
> + case OR_Ambiguous:
> + Diag(Range->getLocStart(), diag::err_ovl_ambiguous_call)
> + << Best->Function->getName() << Range->getSourceRange();
> + CandidateSet->NoteCandidates(*this, OCD_ViableCandidates,
> + llvm::makeArrayRef(&Range, 1));
> + *CallExpr = ExprError();
> + return FRS_DiagnosticIssued;
> +
> + case OR_Deleted:
> + Diag(Range->getLocStart(), diag::err_ovl_deleted_call)
> + << Range->getSourceRange()
> + << Best->Function->isDeleted()
> + << Best->Function->getName()
> + << getDeletedOrUnavailableSuffix(Best->Function);
> + CandidateSet->NoteCandidates(*this, OCD_AllCandidates,
> + llvm::makeArrayRef(&Range, 1));
> + *CallExpr = ExprError();
> + return FRS_DiagnosticIssued;
> + }
It looks like this is equivalent to calling FinishOverloadedCallExpr in all
cases other than OR_No_Viable_Function.
> @@ -1746,7 +1687,127 @@ Sema::ActOnCXXForRangeStmt(SourceLocation ForLoc,
SourceLocation LParenLoc,
>
> return BuildCXXForRangeStmt(ForLoc, ColonLoc, RangeDecl.get(),
> /*BeginEndDecl=*/0, /*Cond=*/0, /*Inc=*/0,
DS,
> - RParenLoc);
> + RParenLoc, ShouldTryDeref);
> +}
> +
> +// Emit the diagnostic appropriate to the given ForRangeStatus. The other
> +// parameters are used for supplementary information.
> +static void ForRangeDiagnostics(Sema &SemaRef, Sema::ForRangeStatus
Status,
> + Expr *Range, SourceLocation RangeLoc,
> + OverloadCandidateSet *CandidateSet,
> + Sema::BeginEndFunction BEF) {
> + switch (Status) {
> + case Sema::FRS_Success:
> + // It worked; nothing to do.
> + break;
> +
> + case Sema::FRS_DiagnosticIssued:
> + // Diagnostic alread issued; note the range's type.
> + SemaRef.Diag(Range->getLocStart(), diag::note_in_for_range)
> + << RangeLoc << BEF << Range->getType();
> + break;
> +
> + case Sema::FRS_NoViableFunction:
> + SemaRef.Diag(Range->getLocStart(), diag::err_for_range_invalid)
> + << RangeLoc << Range->getType() << BEF;
> + CandidateSet->NoteCandidates(SemaRef, OCD_AllCandidates,
> + llvm::makeArrayRef(&Range,
/*NumArgs=*/1));
> + break;
> +
> + case Sema::FRS_BeginEndMismatch:
> + SemaRef.Diag(RangeLoc, diag::err_for_range_member_begin_end_mismatch)
> + << RangeLoc << Range->getType() << BEF;
> + break;
> + }
> +}
> +
> +/// \brief Create the initialization, compare, and increment steps for
> +/// the range-based for loop expression.
> +/// This function does not handle array-based for loops,
> +/// which are created in Sema::BuildCXXForRangeStmt.
> +///
> +/// \returns a ForRangeStatus indicating success or what kind of error
occurred.
> +/// BeginExpr and EndExpr are set and FRS_Success is returned on success;
> +/// CandidateSet and BEF are set and some non-success value is returned
on
> +/// failure.
> +static Sema::ForRangeStatus BuildNonArrayForRange(Sema &SemaRef, Scope
*S,
> + Expr *BeginRange, Expr
*EndRange,
> + QualType RangeType,
> + VarDecl *BeginVar,
> + VarDecl *EndVar,
> + SourceLocation ColonLoc,
> + OverloadCandidateSet
*CandidateSet,
> + ExprResult *BeginExpr,
> + ExprResult *EndExpr,
> + Sema::BeginEndFunction *BEF)
{
> + DeclarationNameInfo BeginNameInfo(
> + &SemaRef.PP.getIdentifierTable().get("begin"), ColonLoc);
> + DeclarationNameInfo
EndNameInfo(&SemaRef.PP.getIdentifierTable().get("end"),
> + ColonLoc);
> +
> + LookupResult BeginMemberLookup(SemaRef, BeginNameInfo,
> + Sema::LookupMemberName);
> + LookupResult EndMemberLookup(SemaRef, EndNameInfo,
Sema::LookupMemberName);
> +
> + 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.)
> + }
> + } 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.
On Thu, Aug 9, 2012 at 4:07 PM, Sam Panzer <panzer at google.com> wrote:
> Here's the next version!
>
>
> On Wed, Aug 8, 2012 at 3:51 PM, Richard Smith <richard at metafoo.co.uk>wrote:
>
>> > --- a/include/clang/Sema/Sema.h
>> > +++ b/include/clang/Sema/Sema.h
>> > @@ -19,6 +19,7 @@
>> > #include "clang/Sema/AnalysisBasedWarnings.h"
>> > #include "clang/Sema/IdentifierResolver.h"
>> > #include "clang/Sema/ObjCMethodList.h"
>> > +#include "clang/Sema/Overload.h"
>>
>> Can you make BuildOverloadedCallExpr local to SemaOverload.cpp to avoid
>> this include? (This is for OverloadCandidateSet::iterator, I think?)
>>
>
> Only the original BuildOverloadCallExpr needed to be in Sema. The other
> one has been renamed to FinishOverloadedCallExpr and moved to
> SemaOverload.h.
>
>
>>
>> > StmtResult ActOnCXXForRangeStmt(SourceLocation ForLoc,
>> > SourceLocation LParenLoc, Stmt
>> *LoopVar,
>> > SourceLocation ColonLoc, Expr
>> *Collection,
>> > - SourceLocation RParenLoc);
>> > + SourceLocation RParenLoc,
>> > + bool ShouldTryDeref = true);
>> > StmtResult BuildCXXForRangeStmt(SourceLocation ForLoc,
>> > SourceLocation ColonLoc,
>> > Stmt *RangeDecl, Stmt *BeginEndDecl,
>> > Expr *Cond, Expr *Inc,
>> > Stmt *LoopVarDecl,
>> > - SourceLocation RParenLoc);
>> > + SourceLocation RParenLoc,
>> > + bool ShouldTryDeref = true);
>>
>> We shouldn't try dereferencing the argument during template
>> instantiation. Please remove the default argument here and set it to false
>> in TreeTransform.h.
>>
>
> Done. There was another call to ActOnCXXForRangeStmt() in
> Parser::ParseForStmt(), where I set ShouldTryDeref to true.
>
>
>>
>> > StmtResult FinishCXXForRangeStmt(Stmt *ForRange, Stmt *Body);
>> >
>> > + bool FinishForRangeVarDecl(VarDecl *Decl, Expr *Init,
>> > + SourceLocation Loc, int diag);
>> > +
>> > StmtResult ActOnGotoStmt(SourceLocation GotoLoc,
>> > SourceLocation LabelLoc,
>> > LabelDecl *TheDecl);
>> > --- a/lib/Sema/SemaOverload.cpp
>> > +++ b/lib/Sema/SemaOverload.cpp
>> > @@ -11195,6 +11238,102 @@ ExprResult
>> Sema::BuildLiteralOperatorCall(LookupResult &R,
>> > return MaybeBindToTemporary(UDL);
>> > }
>> >
>> > +
>> > +/// Build a call to 'begin' or 'end' for a C++0x for-range statement.
>> If the
>> > +/// given LookupResult is non-empty, it is assumed to describe a
>> member which
>> > +/// will be invoked. Otherwise, the function will be found via argument
>> > +/// dependent lookup.
>> > +/// CallExpr is set to a valid expression and FRS_Success returned on
>> success,
>> > +/// otherwise CallExpr is set to ExprError() and some non-success value
>> > +/// is returned.
>> > +Sema::ForRangeStatus
>> > +Sema::BuildForRangeBeginEndCall(Scope *S, SourceLocation Loc,
>> > + SourceLocation RangeLoc, VarDecl *Decl,
>> > + BeginEndFunction BEF,
>> > + const DeclarationNameInfo &NameInfo,
>> > + LookupResult &MemberLookup,
>> > + OverloadCandidateSet *CandidateSet,
>> > + Expr *Range, ExprResult *CallExpr,
>> > + const FunctionDecl **FoundFunction) {
>>
>> In the cases where this function returns FRS_DiagnosticIssued after
>> producing a diagnostic which isn't for-range specific, it'd be useful for
>> it to issue diag::note_for_range_type like it used to.
>>
>
> Done. The note was extended to include which of begin() and end() was
> problematic.
>
>
>>
>> > + CandidateSet->clear();
>> > + if (!MemberLookup.empty()) {
>> > + ExprResult MemberRef =
>> > + BuildMemberReferenceExpr(Range, Range->getType(), Loc,
>> > + /*IsPtr=*/false, CXXScopeSpec(),
>> > + /*TemplateKWLoc=*/SourceLocation(),
>> > + /*FirstQualifierInScope=*/0,
>> > + MemberLookup,
>> > + /*TemplateArgs=*/0);
>> > + if (MemberRef.isInvalid()) {
>> > + *CallExpr = ExprError();
>> > + return FRS_InvalidFunction;
>>
>> This should be FRS_DiagnosticIssued, I think?
>>
>
> Done. It's not immediately obvious which functions issue
> diagnostics...though seeing which ones you picked out, I can see a pattern.
>
>
>>
>> > + }
>> > + *CallExpr = ActOnCallExpr(S, MemberRef.get(), Loc, MultiExprArg(),
>> Loc, 0);
>> > + if (CallExpr->isInvalid()) {
>> > + *CallExpr = ExprError();
>> > + return FRS_InvalidFunction;
>>
>> This too.
>>
>
> Done.
>
>
>>
>> > + }
>> > + } else {
>> > + UnresolvedSet<0> FoundNames;
>> > + // C++0x [stmt.ranged]p1: For the purposes of this name lookup,
>> namespace
>> > + // std is an associated namespace.
>> > + UnresolvedLookupExpr *Fn =
>> > + UnresolvedLookupExpr::Create(Context, /*NamingClass=*/0,
>> > + NestedNameSpecifierLoc(), NameInfo,
>> > + /*NeedsADL=*/true,
>> /*Overloaded=*/false,
>> > + FoundNames.begin(),
>> FoundNames.end(),
>> > + /*LookInStdNamespace=*/true);
>> > +
>> > + buildOverloadedCallSet(S, Fn, Fn, &Range, 1, Loc, CandidateSet,
>> CallExpr);
>>
>> Do you need to handle the case where this returns true?
>>
>
> I don't *think* so. buildOverloadedCallSet should only return true when it
> the candidate set is empty...but for good measure, I added a check.
>
>
>>
>> > +
>> > + if (CandidateSet->empty()) {
>> > + *CallExpr = ExprError();
>> > + return FRS_InvalidFunction;
>>
>> You can use FRS_NoViableFunction here, and remove FRS_InvalidFunction.
>>
>>
> Done.
>
>
>> > + } else {
>> > + OverloadCandidateSet::iterator Best;
>> > + OverloadingResult OverloadResult =
>> > + CandidateSet->BestViableFunction(*this, Fn->getLocStart(),
>> Best);
>> > + switch (OverloadResult) {
>> > + case OR_Success:
>> > + *CallExpr =
>> > + BuildOverloadedCallExpr(S, Fn, Fn, Loc, &Range, 1, Loc, 0,
>> > + CandidateSet, &Best,
>> OverloadResult,
>> > + /*AllowTypoCorrection=*/false);
>> > + if (CallExpr->isInvalid())
>> > + return FRS_InvalidFunction;
>>
>> This should be FRS_DiagnosticIssued.
>>
>
> Done.
>
>
>>
>> > + break;
>> > +
>> > + case OR_No_Viable_Function:
>> > + *CallExpr = ExprError();
>> > + return FRS_NoViableFunction;
>> > +
>> > + case OR_Ambiguous:
>> > + *FoundFunction = Best->Function;
>> > + *CallExpr = ExprError();
>> > + return FRS_Ambiguous;
>> > +
>> > + case OR_Deleted:
>> > + // FIXME: this case is never actually reached, since
>> ActOnCallExpr
>> > + // detects deleted functions, issues a diagnostic, and returns
>> > + // an invalid ExprResult.
>> > + // As a result, the diagnostics for pointers to types with
>> deleted begin
>> > + // or end functions are not as clear as they could be.
>>
>> I think this comment is stale; the ActOnCallExpr call is in the other arm
>> of this 'if'.
>>
>
> It's not so much stale as incorrect. The case can be reached by using a
> deleted ADL begin or end function, which wasn't tested (test added).
>
> Moved and clarified...and then completely fixed (see my next comment).
>
>
>>
>> I'm also not sure that it's a good idea to try to recover if we find an
>> ambiguous or deleted 'begin' or 'end' function. Those both indicate that
>> the range expression was probably fine, but that range-based for is not
>> supposed to work for this type.
>>
>>
> Okay. I canceled the recovery if a diagnostic was already issued. This
> actually lets me clean up some of the code that actually does issue
> diagnostics, remove a few ForRangeStatus's, and delete an extra function
> parameter. Additionally, deleted functions receive better diagnostics than
> they used to.
>
> > /// FixOverloadedFunctionReference - E is an expression that refers to
>> > /// a C++ overloaded function (possibly with some parentheses and
>> > /// perhaps a '&' around it). We have resolved the overloaded function
>> > diff --git a/lib/Sema/SemaStmt.cpp b/lib/Sema/SemaStmt.cpp
>> > index 9affe98..acc5042 100644
>> > --- a/lib/Sema/SemaStmt.cpp
>> > +++ b/lib/Sema/SemaStmt.cpp
>> [...]
>> > +static Sema::ForRangeStatus BuildNonArrayForRange(Sema &SemaRef, Scope
>> *S,
>> > + Expr *BeginRange, Expr
>> *EndRange,
>> > + QualType RangeType,
>> > + VarDecl *BeginVar,
>> > + VarDecl *EndVar,
>> > + SourceLocation ColonLoc,
>> > + OverloadCandidateSet
>> *CandidateSet,
>> > + ExprResult *BeginExpr,
>> > + ExprResult *EndExpr,
>> > + const FunctionDecl
>> **FoundFunction,
>> > + Sema::BeginEndFunction
>> *BEF) {
>>
> [...]
>>
> > + *BEF = Sema::BEF_begin;
>> > + Sema::ForRangeStatus RangeStatus =
>> > + SemaRef.BuildForRangeBeginEndCall(S, ColonLoc, ColonLoc,
>> BeginVar,
>> > + Sema::BEF_begin, BeginNameInfo,
>> > + BeginMemberLookup,
>> CandidateSet,
>> > + BeginRange, BeginExpr,
>> FoundFunction);
>> > + 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,
>> FoundFunction);
>> > + return RangeStatus;
>>
>> If there's a viable begin(...) but lookup for end(...) fails, it's not
>> likely that there's a problem with the range expression, and we shouldn't
>> try to recover in this case.
>>
>
> Done.
>
>
>> > @@ -1834,62 +1898,72 @@ Sema::BuildCXXForRangeStmt(SourceLocation
>> ForLoc, SourceLocation ColonLoc,
>> > BoundExpr.get());
>> > if (EndExpr.isInvalid())
>> > return StmtError();
>> > - if (FinishForRangeVarDecl(*this, EndVar, EndExpr.get(), ColonLoc,
>> > + if (FinishForRangeVarDecl(EndVar, EndExpr.get(), ColonLoc,
>> >
>> diag::err_for_range_iter_deduction_failure)) {
>> > - NoteForRangeBeginEndFunction(*this, EndExpr.get(), BEF_end);
>> > + NoteForRangeBeginEndFunction(EndExpr.get(), BEF_end);
>> > return StmtError();
>> > }
>> > } else {
>> > - DeclarationNameInfo
>> BeginNameInfo(&PP.getIdentifierTable().get("begin"),
>> > - ColonLoc);
>> > - DeclarationNameInfo
>> EndNameInfo(&PP.getIdentifierTable().get("end"),
>> > - ColonLoc);
>> > -
>> > - LookupResult BeginMemberLookup(*this, BeginNameInfo,
>> LookupMemberName);
>> > - LookupResult EndMemberLookup(*this, EndNameInfo,
>> LookupMemberName);
>> > -
>> > - 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;
>> > - LookupQualifiedName(BeginMemberLookup, D);
>> > - LookupQualifiedName(EndMemberLookup, D);
>> > -
>> > - if (BeginMemberLookup.empty() != EndMemberLookup.empty()) {
>> > - Diag(ColonLoc, diag::err_for_range_member_begin_end_mismatch)
>> > - << RangeType << BeginMemberLookup.empty();
>> > + Expr *BeginRange = BeginRangeRef.get();
>> > + Expr *EndRange = EndRangeRef.get();
>> > + const FunctionDecl *FoundFunction = NULL;
>> > + OverloadCandidateSet CandidateSet(RangeLoc);
>> > + Sema::BeginEndFunction BEFFailure;
>> > + ForRangeStatus RangeStatus =
>> > + BuildNonArrayForRange(*this, S, BeginRange, EndRange,
>> RangeType,
>> > + BeginVar, EndVar, ColonLoc,
>> &CandidateSet,
>> > + &BeginExpr, &EndExpr, &FoundFunction,
>> > + &BEFFailure);
>> > +
>> > + // Attempt to dereference pointers and try again.
>> > + if (RangeStatus != FRS_Success) {
>>
>> We shouldn't attempt recovery if we've issued a diagnostic already.
>>
>
> Done, as noted above.
>
>
>>
>> > + if (!ShouldTryDeref)
>> > + return StmtError();
>> > + if (!RangeType->isPointerType()) {
>> > + const ExprResult & TheCall = BEFFailure ? EndExpr: BeginExpr;
>> > + ForRangeDiagnostics(*this, RangeStatus,
>> > + BEFFailure ? EndRange : BeginRange,
>> > + TheCall, FoundFunction,
>> > + RangeLoc, &CandidateSet, BEFFailure);
>> > return StmtError();
>> > }
>> > - } 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.
>> > - }
>> >
>> > - BeginExpr = BuildForRangeBeginEndCall(*this, S, ColonLoc,
>> BeginVar,
>> > - BEF_begin, BeginNameInfo,
>> > - BeginMemberLookup,
>> > - BeginRangeRef.get());
>> > - if (BeginExpr.isInvalid())
>> > - return StmtError();
>> > -
>> > - EndExpr = BuildForRangeBeginEndCall(*this, S, ColonLoc, EndVar,
>> > - BEF_end, EndNameInfo,
>> > - EndMemberLookup,
>> EndRangeRef.get());
>> > - if (EndExpr.isInvalid())
>> > - return StmtError();
>> > + ExprResult DereferencedRange =
>> > + CreateBuiltinUnaryOp(RangeLoc, UO_Deref, Range);
>> > +
>> > + // FIXME: The second ForLoc should really be LParenLoc, though
>> > + // this is only used in one diagnostic in ActOnCXXForRangeStmt.
>> > + StmtResult SR = ActOnCXXForRangeStmt(ForLoc, ForLoc,
>> LoopVarDecl,
>> > + ColonLoc,
>> DereferencedRange.get(),
>> > + RParenLoc, false);
>>
>> It's not great that we produce diagnostics here, and it's doubly not
>> great that we do so before explaining why we're trying to use *expr as a
>> range. Have you considered trapping these diagnostics somehow?
>> DiagnoseInvalidRedeclaration in SemaDecl.cpp uses SFINAETrap for this. With
>> that change, you should be able to make all cases other than
>> FRS_NoViableFunction issue their diagnostics immediately, which should
>> simplify things somewhat. You could also support calling an overloaded
>> operator* by such a technique.
>>
>
> SFINAETrap definitely cleans up the diagnostics orderings; no more
> spurious diagnostics on the dereferenced range! I also discovered
> Sema::BuildUnaryOp, which makes it easy to support both builtin and
> overloaded operator '*', and cleans up the code that does emit diagnostics.
>
>
>>
>> On Wed, Aug 8, 2012 at 11:37 AM, Sam Panzer <panzer at google.com> wrote:
>>
>>> Ping
>>>
>>>
>>> On Fri, Jul 27, 2012 at 3:06 PM, Sam Panzer <panzer at google.com> wrote:
>>>
>>>> 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.
>>>>
>>>> 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.
>>>> 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.
>>>>
>>>> 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.
>>>> I included a few test cases that note this behavior and have
>>>> appropriate FIXME's for when it is corrected.
>>>>
>>>> On Thu, Jul 26, 2012 at 2:01 PM, Richard Smith <richard at metafoo.co.uk>wrote:
>>>>
>>>>> Hi Sam,
>>>>>
>>>>> Sorry for the massive delay reviewing this...
>>>>>
>>>>> > diff --git a/include/clang/Basic/DiagnosticSemaKinds.td
>>>>> b/include/clang/Basic/DiagnosticSemaKinds.td
>>>>> > index 52641b8..97b0469 100644
>>>>> > --- a/include/clang/Basic/DiagnosticSemaKinds.td
>>>>> > +++ b/include/clang/Basic/DiagnosticSemaKinds.td
>>>>> > @@ -1391,7 +1391,14 @@ def err_for_range_member_begin_end_mismatch :
>>>>> Error<
>>>>> > "range type %0 has '%select{begin|end}1' member but no
>>>>> '%select{end|begin}1' member">;
>>>>> > def err_for_range_begin_end_types_differ : Error<
>>>>> > "'begin' and 'end' must return the same type (got %0 and %1)">;
>>>>> > +def err_for_range_invalid: Error<
>>>>> > + "invalid range expression of type %0">;
>>>>> > +def note_for_range_adl_failure: Note<
>>>>> > + "no viable '%select{begin|end}0' function for range of type %1">;
>>>>>
>>>>> Please merge these into a single diagnostic (use the text of
>>>>> note_for_range_adl_failure as the error message).
>>>>>
>>>>
>>>> Done.
>>>>
>>>>
>>>>>
>>>>> > def note_for_range_type : Note<"range has type %0">;
>>>>> > +def err_for_range_dereference : Error<
>>>>> > + "invalid range expression of type %0; did you mean to dereference
>>>>> it "
>>>>> > + "with '*'?">;
>>>>> > def note_for_range_begin_end : Note<
>>>>> > "selected '%select{begin|end}0' %select{function|template }1%2
>>>>> with iterator type %3">;
>>>>> >
>>>>> > diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h
>>>>> > index a48cde0..29fbded 100644
>>>>> > --- a/include/clang/Sema/Sema.h
>>>>> > +++ b/include/clang/Sema/Sema.h
>>>>> > @@ -1923,7 +1923,24 @@ public:
>>>>> > Expr **Args, unsigned NumArgs,
>>>>> > SourceLocation RParenLoc,
>>>>> > Expr *ExecConfig,
>>>>> > - bool AllowTypoCorrection=true);
>>>>> > + OverloadCandidateSet
>>>>> *CandidateSet,
>>>>> > + bool AllowTypoCorrection,
>>>>> > + bool InRangeExpr);
>>>>> > +
>>>>> > + ExprResult BuildOverloadedCallExpr(Scope *S, Expr *Fn,
>>>>> > + UnresolvedLookupExpr *ULE,
>>>>> > + SourceLocation LParenLoc,
>>>>> > + Expr **Args, unsigned NumArgs,
>>>>> > + SourceLocation RParenLoc,
>>>>> > + Expr *ExecConfig,
>>>>> > + bool AllowTypoCorrection=true,
>>>>> > + bool InRangeExpr=false);
>>>>>
>>>>> I don't like passing InRangeExpr in here. It's too specific. It's also
>>>>> unnecessary (more on that later).
>>>>>
>>>>
>>>> > +
>>>>> > +
>>>>> > +// FIXME: Update this stale comment
>>>>>
>>>>> Please do :)
>>>>>
>>>>
>>>> Done. The next comment too :)
>>>>
>>>>
>>>>>
>>>>> > +/// ResolveOverloadedCallFn - Given the call expression that calls
>>>>> Fn
>>>>> > +/// (which eventually refers to the declaration Func) and the call
>>>>> > +/// arguments Args/NumArgs, attempt to resolve the function call
>>>>> down
>>>>> > +/// to a specific function. If overload resolution succeeds, returns
>>>>> > +/// the function declaration produced by overload
>>>>> > +/// resolution. Otherwise, emits diagnostics, deletes all of the
>>>>> > +/// arguments and Fn, and returns NULL.
>>>>> > +
>>>>> > +ExprResult Sema::BuildOverloadedCallExpr(Scope *S, Expr *Fn,
>>>>> > + UnresolvedLookupExpr *ULE,
>>>>> > + SourceLocation LParenLoc,
>>>>> > + Expr **Args, unsigned
>>>>> NumArgs,
>>>>> > + SourceLocation RParenLoc,
>>>>> > + Expr *ExecConfig,
>>>>> > + OverloadCandidateSet
>>>>> *CandidateSet,
>>>>> > + bool AllowTypoCorrection,
>>>>> > + bool InRangeExpr) {
>>>>> > + if (CandidateSet->empty())
>>>>> > return BuildRecoveryCallExpr(*this, S, Fn, ULE, LParenLoc,
>>>>> > llvm::MutableArrayRef<Expr
>>>>> *>(Args, NumArgs),
>>>>> > RParenLoc, /*EmptyLookup=*/true,
>>>>> > AllowTypoCorrection);
>>>>> > - }
>>>>> > -
>>>>> > - UnbridgedCasts.restore();
>>>>> >
>>>>> > OverloadCandidateSet::iterator Best;
>>>>> > - switch (CandidateSet.BestViableFunction(*this, Fn->getLocStart(),
>>>>> Best)) {
>>>>> > + switch (CandidateSet->BestViableFunction(*this,
>>>>> Fn->getLocStart(), Best)) {
>>>>> > case OR_Success: {
>>>>> > FunctionDecl *FDecl = Best->Function;
>>>>> > MarkFunctionReferenced(Fn->getExprLoc(), FDecl);
>>>>> > @@ -9770,6 +9773,10 @@ Sema::BuildOverloadedCallExpr(Scope *S, Expr
>>>>> *Fn, UnresolvedLookupExpr *ULE,
>>>>> > }
>>>>> >
>>>>> > case OR_No_Viable_Function: {
>>>>> > + // In case we're in a range expression, let the range handling
>>>>> code take
>>>>> > + // care of it.
>>>>> > + if (InRangeExpr)
>>>>> > + break;
>>>>>
>>>>> 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?
>>>>>
>>>>
>>>> Nope, it's gone now.
>>>>
>>>> > +/// This version of BuildOverloadedCallExpr creates an
>>>>> OverloadCandidateSet
>>>>> > +/// first.
>>>>> > +ExprResult Sema::BuildOverloadedCallExpr(Scope *S, Expr *Fn,
>>>>> > + UnresolvedLookupExpr *ULE,
>>>>> > + SourceLocation LParenLoc,
>>>>> > + Expr **Args, unsigned
>>>>> NumArgs,
>>>>> > + SourceLocation RParenLoc,
>>>>> > + Expr *ExecConfig,
>>>>> > + bool AllowTypoCorrection,
>>>>> > + bool InRangeExpr) {
>>>>> > +#ifndef NDEBUG
>>>>> > + if (ULE->requiresADL()) {
>>>>> > + // To do ADL, we must have found an unqualified name.
>>>>> > + assert(!ULE->getQualifier() && "qualified name with ADL");
>>>>> > +
>>>>> > + // We don't perform ADL for implicit declarations of builtins.
>>>>> > + // Verify that this was correctly set up.
>>>>> > + FunctionDecl *F;
>>>>> > + if (ULE->decls_begin() + 1 == ULE->decls_end() &&
>>>>> > + (F = dyn_cast<FunctionDecl>(*ULE->decls_begin())) &&
>>>>> > + F->getBuiltinID() && F->isImplicit())
>>>>> > + llvm_unreachable("performing ADL for builtin");
>>>>> > +
>>>>> > + // We don't perform ADL in C.
>>>>> > + assert(getLangOpts().CPlusPlus && "ADL enabled in C");
>>>>> > + } else
>>>>> > + assert(!ULE->isStdAssociatedNamespace() &&
>>>>> > + "std is associated namespace but not doing ADL");
>>>>> > +#endif
>>>>>
>>>>> This checking belongs in buildOverloadedCallSet. We want to perform
>>>>> these checks for range-based for calls too.
>>>>>
>>>>
>>>> Done.
>>>>
>>>>
>>>>>
>>>>> > +
>>>>> > + OverloadCandidateSet CandidateSet(Fn->getExprLoc());
>>>>> > + ExprResult result;
>>>>> > + if (buildOverloadedCallSet(S, Fn, ULE, Args, NumArgs, LParenLoc,
>>>>> > + &CandidateSet, &result))
>>>>> > + return result;
>>>>> > +
>>>>> > + return BuildOverloadedCallExpr(S, Fn, ULE, LParenLoc, Args,
>>>>> NumArgs,
>>>>> > + RParenLoc, ExecConfig,
>>>>> &CandidateSet,
>>>>> > + AllowTypoCorrection, InRangeExpr);
>>>>> > +
>>>>> > }
>>>>> >
>>>>> > static bool IsOverloaded(const UnresolvedSetImpl &Functions) {
>>>>> > diff --git a/lib/Sema/SemaStmt.cpp b/lib/Sema/SemaStmt.cpp
>>>>> > index 9be1d34..18bcf89 100644
>>>>> > --- a/lib/Sema/SemaStmt.cpp
>>>>> > +++ b/lib/Sema/SemaStmt.cpp
>>>>> > @@ -1578,18 +1578,35 @@ void NoteForRangeBeginEndFunction(Sema
>>>>> &SemaRef, Expr *E,
>>>>> > << BEF << IsTemplate << Description << E->getType();
>>>>> > }
>>>>> >
>>>>> > +
>>>>> > +enum ForRangeStatus {
>>>>> > + FRS_Success,
>>>>> > + FRS_InvalidMemberFunction,
>>>>> > + FRS_NoOverload,
>>>>> > + FRS_DeletedOrAmbiguous,
>>>>> > + FRS_NoViableFunction,
>>>>> > + FRS_InvalidIteratorType,
>>>>> > + FRS_BeginEndMismatch
>>>>> > +};
>>>>> > +
>>>>> > /// Build a call to 'begin' or 'end' for a C++0x for-range
>>>>> statement. If the
>>>>> > /// given LookupResult is non-empty, it is assumed to describe a
>>>>> member which
>>>>> > /// will be invoked. Otherwise, the function will be found via
>>>>> argument
>>>>> > /// dependent lookup.
>>>>> > -static ExprResult BuildForRangeBeginEndCall(Sema &SemaRef, Scope
>>>>> *S,
>>>>> > - SourceLocation Loc,
>>>>> > - VarDecl *Decl,
>>>>> > - BeginEndFunction BEF,
>>>>> > - const
>>>>> DeclarationNameInfo &NameInfo,
>>>>> > - LookupResult
>>>>> &MemberLookup,
>>>>> > - Expr *Range) {
>>>>> > - ExprResult CallExpr;
>>>>> > +/// CallExpr is set and FRS_Success returned on success, otherwise
>>>>> some
>>>>> > +/// non-success value is returned.
>>>>> > +static ForRangeStatus
>>>>> > +BuildForRangeBeginEndCall(Sema &SemaRef, Scope *S,
>>>>> > + SourceLocation Loc,
>>>>> > + SourceLocation RangeLoc,
>>>>> > + VarDecl *Decl,
>>>>> > + BeginEndFunction BEF,
>>>>> > + const DeclarationNameInfo &NameInfo,
>>>>> > + LookupResult &MemberLookup,
>>>>> > + OverloadCandidateSet *CandidateSet,
>>>>> > + Expr *Range,
>>>>> > + ExprResult *CallExpr) {
>>>>>
>>>>> Now this is performing the call to BestViableFunction itself, this
>>>>> really belongs in SemaOverload.cpp.
>>>>>
>>>>>
>>>> Done. It took some rearranging :)
>>>> 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.
>>>>
>>>>
>>>>> > + CandidateSet->clear();
>>>>> > if (!MemberLookup.empty()) {
>>>>> > ExprResult MemberRef =
>>>>> > SemaRef.BuildMemberReferenceExpr(Range, Range->getType(), Loc,
>>>>> > @@ -1598,12 +1615,16 @@ static ExprResult
>>>>> BuildForRangeBeginEndCall(Sema &SemaRef, Scope *S,
>>>>> > /*FirstQualifierInScope=*/0,
>>>>> > MemberLookup,
>>>>> > /*TemplateArgs=*/0);
>>>>> > - if (MemberRef.isInvalid())
>>>>> > - return ExprError();
>>>>> > - CallExpr = SemaRef.ActOnCallExpr(S, MemberRef.get(), Loc,
>>>>> MultiExprArg(),
>>>>> > + if (MemberRef.isInvalid()) {
>>>>> > + *CallExpr = ExprError();
>>>>> > + return FRS_InvalidMemberFunction;
>>>>> > + }
>>>>> > + *CallExpr = SemaRef.ActOnCallExpr(S, MemberRef.get(), Loc,
>>>>> MultiExprArg(),
>>>>> > Loc, 0);
>>>>> > - if (CallExpr.isInvalid())
>>>>> > - return ExprError();
>>>>> > + if (CallExpr->isInvalid()) {
>>>>> > + *CallExpr = ExprError();
>>>>> > + return FRS_InvalidMemberFunction;
>>>>> > + }
>>>>> > } else {
>>>>> > UnresolvedSet<0> FoundNames;
>>>>> > // C++0x [stmt.ranged]p1: For the purposes of this name lookup,
>>>>> namespace
>>>>> > @@ -1614,20 +1635,49 @@ static ExprResult
>>>>> BuildForRangeBeginEndCall(Sema &SemaRef, Scope *S,
>>>>> > /*NeedsADL=*/true,
>>>>> /*Overloaded=*/false,
>>>>> > FoundNames.begin(),
>>>>> FoundNames.end(),
>>>>> > /*LookInStdNamespace=*/true);
>>>>> > - CallExpr = SemaRef.BuildOverloadedCallExpr(S, Fn, Fn, Loc,
>>>>> &Range, 1, Loc,
>>>>> > - 0,
>>>>> /*AllowTypoCorrection=*/false);
>>>>> > - if (CallExpr.isInvalid()) {
>>>>> > - SemaRef.Diag(Range->getLocStart(), diag::note_for_range_type)
>>>>> > - << Range->getType();
>>>>> > - return ExprError();
>>>>> > +
>>>>> > + SemaRef.buildOverloadedCallSet(S, Fn, Fn, &Range, 1, Loc,
>>>>> CandidateSet,
>>>>> > + CallExpr);
>>>>> > +
>>>>> > + OverloadCandidateSet::iterator Best;
>>>>> > + if (CandidateSet->empty()) {
>>>>> > + *CallExpr = ExprError();
>>>>> > + return FRS_NoOverload;
>>>>> > + } else {
>>>>> > + switch (CandidateSet->BestViableFunction(SemaRef,
>>>>> Fn->getLocStart(),
>>>>> > + Best)) {
>>>>> > + case OR_Success:
>>>>> > + *CallExpr =
>>>>> > + SemaRef.BuildOverloadedCallExpr(S, Fn, Fn, Loc, &Range,
>>>>> 1, Loc, 0,
>>>>> > + CandidateSet,
>>>>> > +
>>>>> /*AllowTypoCorrection=*/false,
>>>>> > + /*InRangeExpr=*/true);
>>>>>
>>>>> 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?
>>>>>
>>>>
>>>> Done.
>>>>
>>>>
>>>>>
>>>>> > + if (CallExpr->isInvalid())
>>>>> > + return FRS_NoOverload;
>>>>> > + break;
>>>>> > +
>>>>> > + case OR_No_Viable_Function:
>>>>> > + *CallExpr = ExprError();
>>>>> > + return FRS_NoViableFunction;
>>>>> > +
>>>>> > + case OR_Deleted:
>>>>> > + case OR_Ambiguous:
>>>>> > + // Here we want to defer to the diagnostics in
>>>>> BuildOverloadedCallExpr.
>>>>> > + // The return value doesn't matter, since we're returning
>>>>> an ExprError()
>>>>> > + // anyway.
>>>>> > + SemaRef.BuildOverloadedCallExpr(S, Fn, Fn, Loc, &Range, 1,
>>>>> Loc, 0,
>>>>> > +
>>>>> /*AllowTypoCorrection=*/false,
>>>>> > + /*InRangeExpr=*/true);
>>>>> > + *CallExpr = ExprError();
>>>>> > + return FRS_DeletedOrAmbiguous;
>>>>>
>>>>> 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?
>>>>>
>>>>
>>>> I was looking to avoid duplicating the code that emits the diagnostic,
>>>> but the cure was worse. Done.
>>>>
>>>>
>>>>>
>>>>> > + }
>>>>> > }
>>>>> > +
>>>>> > }
>>>>> > - if (FinishForRangeVarDecl(SemaRef, Decl, CallExpr.get(), Loc,
>>>>> > -
>>>>> diag::err_for_range_iter_deduction_failure)) {
>>>>> > - NoteForRangeBeginEndFunction(SemaRef, CallExpr.get(), BEF);
>>>>> > - return ExprError();
>>>>> > - }
>>>>> > - return CallExpr;
>>>>> > + if (FinishForRangeVarDecl(SemaRef, Decl, CallExpr->get(), Loc,
>>>>> > +
>>>>> diag::err_for_range_iter_deduction_failure))
>>>>> > + return FRS_InvalidIteratorType;
>>>>> > + return FRS_Success;
>>>>> > }
>>>>> >
>>>>> > }
>>>>> > @@ -1700,6 +1750,129 @@ Sema::ActOnCXXForRangeStmt(SourceLocation
>>>>> ForLoc, SourceLocation LParenLoc,
>>>>> > RParenLoc);
>>>>> > }
>>>>> >
>>>>> > +// Emit the diagnostic appropriate to the given ForRangeStatus. The
>>>>> other
>>>>> > +// parameters are used for supplementary information.
>>>>> > +static void ForRangeDiagnostics(Sema &SemaRef, ForRangeStatus st,
>>>>> Expr *Range,
>>>>> > + const ExprResult &CallExpr,
>>>>> > + SourceLocation RangeLoc,
>>>>> > + OverloadCandidateSet *CandidateSet,
>>>>> > + BeginEndFunction BEF) {
>>>>> > + switch (st) {
>>>>> > + case FRS_Success:
>>>>> > + // It worked; nothing to do.
>>>>> > + break;
>>>>> > +
>>>>> > + case FRS_InvalidMemberFunction:
>>>>> > + SemaRef.Diag(Range->getLocStart(), diag::err_for_range_invalid)
>>>>> > + << RangeLoc << Range->getType();
>>>>> > + SemaRef.Diag(Range->getLocStart(),
>>>>> diag::note_for_range_adl_failure)
>>>>> > + << BEF << Range->getType();
>>>>> > + break;
>>>>> > +
>>>>> > + case FRS_NoOverload:
>>>>> > + SemaRef.Diag(Range->getLocStart(), diag::err_for_range_invalid)
>>>>> > + << RangeLoc << Range->getType();
>>>>> > + SemaRef.Diag(Range->getLocStart(),
>>>>> diag::note_for_range_adl_failure)
>>>>> > + << BEF << Range->getType();
>>>>> > + break;
>>>>> > +
>>>>> > + case FRS_DeletedOrAmbiguous:
>>>>> > + // Some of the diagnostics are emitted in
>>>>> BuildOverloadedCallExpr, so we
>>>>> > + // just add these.
>>>>> > + SemaRef.Diag(Range->getLocStart(), diag::err_for_range_invalid)
>>>>> > + << RangeLoc << Range->getType();
>>>>> > + SemaRef.Diag(Range->getLocStart(),
>>>>> diag::note_for_range_adl_failure)
>>>>> > + << BEF << Range->getType();
>>>>> > + break;
>>>>> > +
>>>>> > + case FRS_NoViableFunction:
>>>>> > + SemaRef.Diag(Range->getLocStart(), diag::err_for_range_invalid)
>>>>> > + << RangeLoc << Range->getType();
>>>>> > + CandidateSet->NoteCandidates(SemaRef, OCD_AllCandidates,
>>>>> > + llvm::makeArrayRef(&Range,
>>>>> /*NumArgs=*/1));
>>>>> > + SemaRef.Diag(Range->getLocStart(),
>>>>> diag::note_for_range_adl_failure)
>>>>> > + << BEF << Range->getType();
>>>>> > + break;
>>>>> > +
>>>>> > + case FRS_InvalidIteratorType:
>>>>> > + NoteForRangeBeginEndFunction(SemaRef, CallExpr.get(), BEF);
>>>>> > + break;
>>>>> > +
>>>>> > + case FRS_BeginEndMismatch:
>>>>> > + SemaRef.Diag(RangeLoc,
>>>>> diag::err_for_range_member_begin_end_mismatch)
>>>>> > + << Range->getType() << BEF;
>>>>> > + break;
>>>>> > + }
>>>>> > +}
>>>>>
>>>>> 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:
>>>>>
>>>>>
>>>> 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.
>>>>>
>>>>
>>>> Done. This required that I move NoteForRangeBeginEndFunction() into
>>>> Sema so that BuildForRangeBeginEndCall() could call it.
>>>>
>>>>
>>>>>
>>>>> > +
>>>>> > +/// \brief Create the initialization, compare, and increment steps
>>>>> for
>>>>> > +/// the range-based for loop expression.
>>>>> > +/// This function does not handle array-based for loops,
>>>>> > +/// which are created in Sema::BuildCXXForRangeStmt.
>>>>> > +///
>>>>> > +/// \returns a ForRangeStatus indicating success or what kind of
>>>>> error occurred.
>>>>> > +/// BeginExpr and EndExpr are set and FRS_Success is returned on
>>>>> success;
>>>>> > +/// CandidateSet and BEF are set and some non-success value is
>>>>> returned on
>>>>> > +/// failure.
>>>>> > +static ForRangeStatus BuildNonArrayForRange(Sema &SemaRef, Scope *S,
>>>>> > + Expr *BeginRange, Expr
>>>>> *EndRange,
>>>>> > + QualType RangeType,
>>>>> > + VarDecl *BeginVar,
>>>>> > + VarDecl *EndVar,
>>>>> > + SourceLocation ColonLoc,
>>>>> > + OverloadCandidateSet
>>>>> *CandidateSet,
>>>>> > + ExprResult *BeginExpr,
>>>>> > + ExprResult *EndExpr,
>>>>> > + BeginEndFunction *BEF) {
>>>>> > + DeclarationNameInfo BeginNameInfo(
>>>>> > + &SemaRef.PP.getIdentifierTable().get("begin"), ColonLoc);
>>>>> > + DeclarationNameInfo
>>>>> EndNameInfo(&SemaRef.PP.getIdentifierTable().get("end"),
>>>>> > + ColonLoc);
>>>>> > +
>>>>> > + LookupResult BeginMemberLookup(SemaRef, BeginNameInfo,
>>>>> > + Sema::LookupMemberName);
>>>>> > + LookupResult EndMemberLookup(SemaRef, EndNameInfo,
>>>>> Sema::LookupMemberName);
>>>>> > +
>>>>> > + 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() ? BEF_end : BEF_begin;
>>>>> > + return FRS_BeginEndMismatch;
>>>>> > + }
>>>>> > + } 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 = BEF_begin;
>>>>> > + ForRangeStatus
>>>>> > + RangeStatus = BuildForRangeBeginEndCall(SemaRef, S, ColonLoc,
>>>>> > + ColonLoc, BeginVar,
>>>>> > + BEF_begin,
>>>>> BeginNameInfo,
>>>>> > + BeginMemberLookup,
>>>>> > + CandidateSet,
>>>>> > + BeginRange,
>>>>> BeginExpr);
>>>>> > + if (RangeStatus != FRS_Success)
>>>>> > + return st;
>>>>> > +
>>>>> > + *BEF = BEF_end;
>>>>> > + RangeStatus = BuildForRangeBeginEndCall(SemaRef, S, ColonLoc,
>>>>> ColonLoc,
>>>>> > + EndVar, BEF_end,
>>>>> EndNameInfo,
>>>>> > + EndMemberLookup,
>>>>> CandidateSet,
>>>>> > + EndRange, EndExpr);
>>>>> > + return RangeStatus;
>>>>> > +}
>>>>> > +
>>>>> > /// BuildCXXForRangeStmt - Build or instantiate a C++0x for-range
>>>>> statement.
>>>>> > StmtResult
>>>>> > Sema::BuildCXXForRangeStmt(SourceLocation ForLoc, SourceLocation
>>>>> ColonLoc,
>>>>> > @@ -1791,49 +1964,58 @@ Sema::BuildCXXForRangeStmt(SourceLocation
>>>>> ForLoc, SourceLocation ColonLoc,
>>>>> > return StmtError();
>>>>> > }
>>>>> > } else {
>>>>> > - DeclarationNameInfo
>>>>> BeginNameInfo(&PP.getIdentifierTable().get("begin"),
>>>>> > - ColonLoc);
>>>>> > - DeclarationNameInfo
>>>>> EndNameInfo(&PP.getIdentifierTable().get("end"),
>>>>> > - ColonLoc);
>>>>> > -
>>>>> > - LookupResult BeginMemberLookup(*this, BeginNameInfo,
>>>>> LookupMemberName);
>>>>> > - LookupResult EndMemberLookup(*this, EndNameInfo,
>>>>> LookupMemberName);
>>>>> > -
>>>>> > - 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;
>>>>> > - LookupQualifiedName(BeginMemberLookup, D);
>>>>> > - LookupQualifiedName(EndMemberLookup, D);
>>>>> > -
>>>>> > - if (BeginMemberLookup.empty() != EndMemberLookup.empty()) {
>>>>> > - Diag(ColonLoc,
>>>>> diag::err_for_range_member_begin_end_mismatch)
>>>>> > - << RangeType << BeginMemberLookup.empty();
>>>>> > + Expr *BeginRange = BeginRangeRef.get();
>>>>> > + Expr *EndRange = EndRangeRef.get();
>>>>> > + OverloadCandidateSet CandidateSet(RangeLoc);
>>>>> > + BeginEndFunction BEFFailure;
>>>>> > + ForRangeStatus RangeStatus =
>>>>> > + BuildNonArrayForRange(*this, S, BeginRange, EndRange,
>>>>> RangeType,
>>>>> > + BeginVar, EndVar, ColonLoc,
>>>>> &CandidateSet,
>>>>> > + &BeginExpr, &EndExpr, &BEFFailure);
>>>>> > +
>>>>> > + // Attempt to dereference pointers and try again.
>>>>> > + if (RangeStatus != FRS_Success) {
>>>>> > + if (!RangeType->isPointerType()) {
>>>>> > + const ExprResult & TheCall = BEFFailure ? EndExpr:
>>>>> BeginExpr;
>>>>> > + ForRangeDiagnostics(*this, RangeStatus,
>>>>> > + BEFFailure ? EndRange : BeginRange,
>>>>> > + TheCall, RangeLoc, &CandidateSet,
>>>>> BEFFailure);
>>>>> > return StmtError();
>>>>> > }
>>>>> > - } 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.
>>>>> > + BeginRange =
>>>>> > + new (Context) UnaryOperator(BeginRange, UO_Deref,
>>>>> > + RangeType->getPointeeType(),
>>>>> > + VK_LValue, OK_Ordinary,
>>>>> RangeLoc);
>>>>> > + EndRange =
>>>>> > + new (Context) UnaryOperator(EndRange, UO_Deref,
>>>>> > + RangeType->getPointeeType(),
>>>>> > + VK_LValue, OK_Ordinary,
>>>>> RangeLoc);
>>>>> > +
>>>>> > + BeginEndFunction DerefBEF;
>>>>> > + ForRangeStatus DerefStatus
>>>>> > + = BuildNonArrayForRange(*this, S, BeginRange, EndRange,
>>>>> > + BeginRange->getType(),
>>>>> > + BeginVar, EndVar,
>>>>> > + ColonLoc, &CandidateSet,
>>>>> > + &BeginExpr, &EndExpr,
>>>>> &DerefBEF);
>>>>> > + if (DerefStatus != FRS_Success) {
>>>>> > + const ExprResult & TheCall = BEFFailure ? EndExpr:
>>>>> BeginExpr;
>>>>> > + ForRangeDiagnostics(*this, RangeStatus,
>>>>> > + DerefBEF ? EndRangeRef.get():
>>>>> BeginRangeRef.get(),
>>>>> > + TheCall, RangeLoc, &CandidateSet,
>>>>> BEFFailure);
>>>>> > + return StmtError();
>>>>> > + } else {
>>>>> > + // The attempt to dereference would likely succeed.
>>>>> > + Diag(RangeLoc, diag::err_for_range_dereference)
>>>>> > + << RangeLoc << RangeType
>>>>> > + << FixItHint::CreateInsertion(RangeLoc, "*");
>>>>>
>>>>> 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.
>>>>>
>>>>
>>>> I changed it to rebuild the correct expression here by introducing
>>>> mutual recursion with ActOnCXXForRangeStmt, limiting the depth to one.
>>>>
>>>>
>>>>>
>>>>> > + }
>>>>> > }
>>>>> > -
>>>>> > - BeginExpr = BuildForRangeBeginEndCall(*this, S, ColonLoc,
>>>>> BeginVar,
>>>>> > - BEF_begin,
>>>>> BeginNameInfo,
>>>>> > - BeginMemberLookup,
>>>>> > - BeginRangeRef.get());
>>>>> > - if (BeginExpr.isInvalid())
>>>>> > - return StmtError();
>>>>> > -
>>>>> > - EndExpr = BuildForRangeBeginEndCall(*this, S, ColonLoc,
>>>>> EndVar,
>>>>> > - BEF_end, EndNameInfo,
>>>>> > - EndMemberLookup,
>>>>> EndRangeRef.get());
>>>>> > - if (EndExpr.isInvalid())
>>>>> > - return StmtError();
>>>>> > }
>>>>> >
>>>>> > + assert(!BeginExpr.isInvalid() && !EndExpr.isInvalid() &&
>>>>> > + "invalid range expression in for loop");
>>>>> > +
>>>>> > // C++0x [decl.spec.auto]p6: BeginType and EndType must be the
>>>>> same.
>>>>> > QualType BeginType = BeginVar->getType(), EndType =
>>>>> EndVar->getType();
>>>>> > if (!Context.hasSameType(BeginType, EndType)) {
>>>>> > diff --git a/test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp
>>>>> b/test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp
>>>>> > index 96bb472..e5809f0 100644
>>>>> > --- a/test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp
>>>>> > +++ b/test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp
>>>>> > @@ -3,20 +3,21 @@
>>>>> > struct pr12960 {
>>>>> > int begin;
>>>>> > void foo(int x) {
>>>>> > - for (int& it : x) { // expected-error {{use of undeclared
>>>>> identifier 'begin'}} expected-note {{range has type 'int'}}
>>>>> > + for (int& it : x) { //expected-error{{invalid range
>>>>> expression}}\
>>>>> > + expected-note{{no viable 'begin' function for range of type
>>>>> 'int'}}
>>>>> > }
>>>>> > }
>>>>> > };
>>>>> >
>>>>> > namespace std {
>>>>> > template<typename T>
>>>>> > - auto begin(T &&t) -> decltype(t.begin()) { return t.begin(); }
>>>>> // expected-note 4{{ignored: substitution failure}}
>>>>> > + auto begin(T &&t) -> decltype(t.begin()) { return t.begin(); }
>>>>> // expected-note 3{{ignored: substitution failure}}
>>>>> > template<typename T>
>>>>> > auto end(T &&t) -> decltype(t.end()) { return t.end(); } //
>>>>> expected-note {{candidate template ignored: substitution failure [with T =
>>>>> }}
>>>>> >
>>>>> > template<typename T>
>>>>> > auto begin(T &&t) -> decltype(t.alt_begin()) { return
>>>>> t.alt_begin(); } // expected-note {{selected 'begin' template [with T = }} \
>>>>> > -
>>>>> expected-note 4{{candidate template ignored: substitution
>>>>> failure [with T = }}
>>>>> > +
>>>>> expected-note 3{{candidate template ignored: substitution
>>>>> failure [with T = }}
>>>>> > template<typename T>
>>>>> > auto end(T &&t) -> decltype(t.alt_end()) { return t.alt_end();
>>>>> } // expected-note {{candidate template ignored: substitution failure [with
>>>>> T = }}
>>>>> >
>>>>> > @@ -116,9 +117,11 @@ void g() {
>>>>> > struct NoEndADL {
>>>>> > null_t alt_begin();
>>>>> > };
>>>>> > - for (auto u : NoBeginADL()) { // expected-error {{no matching
>>>>> function for call to 'begin'}} expected-note {{range has type 'NoBeginADL'}}
>>>>> > + for (auto u : NoBeginADL()) {// expected-error{{invalid range
>>>>> expression of type 'NoBeginADL'}} \
>>>>> > + expected-note{{no viable 'begin' function for range of type
>>>>> 'NoBeginADL'}}
>>>>> > }
>>>>> > - for (auto u : NoEndADL()) { // expected-error {{no matching
>>>>> function for call to 'end'}} expected-note {{range has type 'NoEndADL'}}
>>>>> > + for (auto u : NoEndADL()) { // expected-error{{invalid range
>>>>> expression of type 'NoEndADL'}} \
>>>>> > + expected-note{{no viable 'end' function for range of type
>>>>> 'NoEndADL'}}
>>>>> > }
>>>>> >
>>>>> > struct NoBegin {
>>>>> > @@ -156,8 +159,8 @@ void g() {
>>>>> > for (int n : NoCopy()) { // ok
>>>>> > }
>>>>> >
>>>>> > - for (int n : 42) { // expected-error {{no matching function for
>>>>> call to 'begin'}} \
>>>>> > - expected-note {{range has type 'int'}}
>>>>> > + for (int n : 42) { // expected-error{{invalid range expression of
>>>>> type 'int'}} \
>>>>> > + expected-note{{no viable 'begin' function for range of type
>>>>> 'int'}}
>>>>> > }
>>>>> >
>>>>> > for (auto a : *also_incomplete) { // expected-error {{cannot use
>>>>> incomplete type 'struct Incomplete' as a range}}
>>>>> > @@ -179,9 +182,10 @@ template void h<A(&)[13], int>(A(&)[13]); //
>>>>> expected-note {{requested here}}
>>>>> >
>>>>> > template<typename T>
>>>>> > void i(T t) {
>>>>> > - for (auto u : t) { // expected-error {{no matching function for
>>>>> call to 'begin'}} \
>>>>> > - expected-error {{member function 'begin'
>>>>> not viable}} \
>>>>> > - expected-note {{range has type}}
>>>>> > + for (auto u : t) { // expected-error {{invalid range expression
>>>>> of type 'A *'; did you mean to dereference it with '*'?}} \
>>>>> > + expected-error {{member function 'begin'
>>>>> not viable}}\
>>>>> > + expected-error{{invalid range expression of
>>>>> type 'const A'}}\
>>>>> > + expected-note{{no viable 'begin' function
>>>>> for range of type 'const A'}}
>>>>> > }
>>>>> > }
>>>>> > template void i<A[13]>(A*); // expected-note {{requested here}}
>>>>> > @@ -204,7 +208,8 @@ void end(VoidBeginADL);
>>>>> > void j() {
>>>>> > for (auto u : NS::ADL()) {
>>>>> > }
>>>>> > - for (auto u : NS::NoADL()) { // expected-error {{no matching
>>>>> function for call to 'begin'}} expected-note {{range has type}}
>>>>> > + for (auto u : NS::NoADL()) { // expected-error{{invalid range
>>>>> expression of type 'NS::NoADL'}}\
>>>>> > + expected-note {{no viable 'begin' function for range of type
>>>>> 'NS::NoADL'}}
>>>>> > }
>>>>> > for (auto a : VoidBeginADL()) { // expected-error {{cannot use
>>>>> type 'void' as an iterator}}
>>>>> > }
>>>>> > @@ -215,4 +220,3 @@ void example() {
>>>>> > for (int &x : array)
>>>>> > x *= 2;
>>>>> > }
>>>>> > -
>>>>> > diff --git a/test/SemaCXX/for-range-no-std.cpp
>>>>> b/test/SemaCXX/for-range-no-std.cpp
>>>>> > index fa42ca4..8f3213b 100644
>>>>> > --- a/test/SemaCXX/for-range-no-std.cpp
>>>>> > +++ b/test/SemaCXX/for-range-no-std.cpp
>>>>> > @@ -31,10 +31,11 @@ NS::iter end(NS::NoADL);
>>>>> > void f() {
>>>>> > int a[] = {1, 2, 3};
>>>>> > for (auto b : S()) {} // ok
>>>>> > - for (auto b : T()) {} // expected-error {{no matching function
>>>>> for call to 'begin'}} expected-note {{range has type}}
>>>>> > + for (auto b : T()) {} // expected-error {{invalid range
>>>>> expression of type 'T'}} expected-note {{no viable 'begin' function for
>>>>> range of type 'T'}}
>>>>> > for (auto b : a) {} // ok
>>>>> > for (int b : NS::ADL()) {} // ok
>>>>> > - for (int b : NS::NoADL()) {} // expected-error {{no matching
>>>>> function for call to 'begin'}} expected-note {{range has type}}
>>>>> > + for (int b : NS::NoADL()) {} // expected-error {{invalid range
>>>>> expression of type 'NS::NoADL'}} \
>>>>> > + expected-note {{no viable 'begin' function for range of type
>>>>> 'NS::NoADL'}}
>>>>> > }
>>>>> >
>>>>> > void PR11601() {
>>>>> > diff --git a/test/SemaCXX/typo-correction.cpp
>>>>> b/test/SemaCXX/typo-correction.cpp
>>>>> > index 893f08a..f8bb8d0 100644
>>>>> > --- a/test/SemaCXX/typo-correction.cpp
>>>>> > +++ b/test/SemaCXX/typo-correction.cpp
>>>>> > @@ -155,7 +155,8 @@ void Test3() {
>>>>> > struct R {};
>>>>> > bool begun(R);
>>>>> > void RangeTest() {
>>>>> > - for (auto b : R()) {} // expected-error {{use of undeclared
>>>>> identifier 'begin'}} expected-note {{range has type}}
>>>>> > + for (auto b : R()) {} // expected-error {{invalid range
>>>>> expression of type 'R'}}\
>>>>> > + expected-note {{no viable 'begin' function for range of type 'R'}}
>>>>> > }
>>>>> >
>>>>> > // PR 12019 - Avoid infinite mutual recursion in
>>>>> DiagnoseInvalidRedeclaration
>>>>> > --
>>>>> > 1.7.7.3
>>>>> >
>>>>>
>>>>> On Mon, Jul 23, 2012 at 3:41 PM, Sam Panzer <panzer at google.com> wrote:
>>>>>
>>>>>> Ping.
>>>>>>
>>>>>>
>>>>>> On Wed, Jul 11, 2012 at 4:29 PM, Sam Panzer <panzer at google.com>wrote:
>>>>>>
>>>>>>> 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.
>>>>>>>
>>>>>>>
>>>>>>> On Mon, Jun 18, 2012 at 9:40 AM, Sam Panzer <panzer at google.com>wrote:
>>>>>>>
>>>>>>>> *sigh*
>>>>>>>> I ran format-patch from the correct branch this time.
>>>>>>>>
>>>>>>>> On Sun, Jun 17, 2012 at 4:49 PM, Richard Smith <
>>>>>>>> richard at metafoo.co.uk> wrote:
>>>>>>>>
>>>>>>>>> On Thu, Jun 14, 2012 at 10:32 AM, Sam Panzer <panzer at google.com>
>>>>>>>>> wrote:
>>>>>>>>> > I will now be including the word 'attach' in these emails as
>>>>>>>>> Gmail's
>>>>>>>>> > equivalent of -Wmissing-patch.
>>>>>>>>>
>>>>>>>>> Sadly that won't save you from running 'diff' in the wrong
>>>>>>>>> checkout.
>>>>>>>>> This is the printf / c_str patch.
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120815/49d8fd0f/attachment.html>
More information about the cfe-commits
mailing list