[cfe-commits] [patch] Better diagnostics in for-range expressions.
Sam Panzer
panzer at google.com
Fri Jul 27 15:06:10 PDT 2012
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/20120727/51a66db1/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: for-range-diagnostic.patch
Type: application/octet-stream
Size: 50562 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120727/51a66db1/attachment.obj>
More information about the cfe-commits
mailing list