[cfe-commits] [patch] Better diagnostics in for-range expressions.
Sam Panzer
panzer at google.com
Wed Aug 8 11:37:58 PDT 2012
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/20120808/794a8934/attachment.html>
More information about the cfe-commits
mailing list