On Tue, Jun 21, 2011 at 11:32 AM, Chandler Carruth <[email protected]>wrote:
> FYI, patch generally looks good. > > I think in two places you're needlessly checking for a reference type: the > base type of a base initializer. > Done. Doug, ok to commit? > On Mon, Jun 20, 2011 at 10:07 AM, Manuel Klimek <[email protected]> wrote: > >> Changed to always require a type in ParenListExpr and tried to fill in the >> right types (sanity checking welcome :) >> >> On Fri, Jun 17, 2011 at 9:15 AM, Douglas Gregor <[email protected]>wrote: >> >>> >>> On Jun 17, 2011, at 8:11 AM, Douglas Gregor wrote: >>> >>> >>> On Jun 16, 2011, at 11:47 PM, Manuel Klimek wrote: >>> >>> On Thu, Jun 16, 2011 at 11:36 PM, Douglas Gregor <[email protected]>wrote: >>> >>>> >>>> On Jun 15, 2011, at 10:11 AM, Manuel Klimek wrote: >>>> >>>> On Tue, Jun 14, 2011 at 8:21 PM, Douglas Gregor <[email protected]>wrote: >>>> >>>>> >>>>> On Jun 14, 2011, at 6:06 PM, Manuel Klimek wrote: >>>>> >>>>> > The attached patch >>>>> > - fixes the introduction of null types for ParenListExpr's that end >>>>> up >>>>> > in the AST for explicit initializers by making the constructor of >>>>> > ParenListExpr take a type (as suggested by dgregor on irc) >>>>> > - gets rid of some code I assume is dead (tested by running the tests >>>>> > and by running it over all of our internal C++ code without hitting >>>>> > any of those asserts - and by my inability to come up with an example >>>>> > that hits that code path (which admittedly doesn't mean a lot)) >>>>> > - tested that the other in-use cases that create ParenListExpr's >>>>> > without a type don't lead to them being in the AST in the end >>>>> >>>>> Index: lib/Sema/SemaDeclCXX.cpp >>>>> diff --git a/lib/Sema/SemaDeclCXX.cpp b/lib/Sema/SemaDeclCXX.cpp >>>>> index >>>>> ce99efbd0bd2020f702da71fc87a80d0b86759a8..a95ef69085526291eb7532aeb9788f68bab235f6 >>>>> 100644 >>>>> --- a/lib/Sema/SemaDeclCXX.cpp >>>>> +++ b/lib/Sema/SemaDeclCXX.cpp >>>>> @@ -1617,7 +1617,7 @@ Sema::BuildMemberInitializer(ValueDecl *Member, >>>>> Expr **Args, >>>>> // Can't check initialization for a member of dependent type or >>>>> when >>>>> // any of the arguments are type-dependent expressions. >>>>> Init = new (Context) ParenListExpr(Context, LParenLoc, Args, >>>>> NumArgs, >>>>> - RParenLoc); >>>>> + RParenLoc, QualType()); >>>>> >>>>> // Erase any temporaries within this evaluation context; we're not >>>>> // going to track them in the AST, since we'll be rebuilding the >>>>> >>>>> Why not just use Member->getType() as the type of the ParenListExpr, so >>>>> it never has a NULL type? >>>>> >>>> >>>> Because I wasn't able to get it to make a difference, in which case I >>>> default to not changing it. If you think the invariant should be that the >>>> ParenListExpr always has a type, I'm happy to change this. I'll have a hard >>>> time writing a test for it, though ... ;) >>>> >>>> >>>> I just figured that if you're going to fix the problem in one place, it >>>> would be better to just make it an AST invariant. >>>> >>>> >>>>> Index: lib/Sema/SemaInit.cpp >>>>> diff --git a/lib/Sema/SemaInit.cpp b/lib/Sema/SemaInit.cpp >>>>> index >>>>> a33f5d0b2f3ef530c689a2ddc28ebaef3074635e..f0d39893a449e62902f21160f10bd6a400353789 >>>>> 100644 >>>>> --- a/lib/Sema/SemaInit.cpp >>>>> +++ b/lib/Sema/SemaInit.cpp >>>>> @@ -3684,19 +3684,9 @@ InitializationSequence::Perform(Sema &S, >>>>> >>>>> } >>>>> } >>>>> - >>>>> - if (Kind.getKind() == InitializationKind::IK_Copy || >>>>> Kind.isExplicitCast()) >>>>> - return ExprResult(Args.release()[0]); >>>>> - >>>>> - if (Args.size() == 0) >>>>> - return S.Owned((Expr *)0); >>>>> - >>>>> - unsigned NumArgs = Args.size(); >>>>> - return S.Owned(new (S.Context) ParenListExpr(S.Context, >>>>> - SourceLocation(), >>>>> - (Expr >>>>> **)Args.release(), >>>>> - NumArgs, >>>>> - SourceLocation())); >>>>> + assert(Kind.getKind() == InitializationKind::IK_Copy || >>>>> + Kind.isExplicitCast()); >>>>> + return ExprResult(Args.release()[0]); >>>>> } >>>>> >>>>> // No steps means no initialization. >>>>> >>>>> Hrm, interesting. This is only dead because its potential callers seem >>>>> to avoid building InitializationSequences in dependent cases. Please >>>>> update >>>>> the assert to always check that Args.size() ==1, and if *that* doesn't >>>>> trigger any failures, this change is okay. We can always resurrect the >>>>> code >>>>> if the callers change. >>>>> >>>> >>>> I'm happy to do this, but I don't understand the underlying assumption >>>> yet - even if Args.size() != 1, if the assert that I added is true, the >>>> code >>>> will behave exactly the same way as before (before we would early return if >>>> it is true). So I'm not sure what the extra assert would change (if it >>>> breaks, and that makes the code wrong, I'd say the code was wrong >>>> before...) >>>> >>>> >>>> The previous code would return *all* of the arguments in Args (NumArgs >>>> of them) as a ParenListExpr, indicating direct initialization. >>>> >>>> The new code only returns the first of the arguments, so if we're going >>>> to make that change, we need to assert that there is exactly one argument. >>>> >>> >>> >>> Now I feel like I'm really missing something here... >>> In the previous code: >>> >>> if (Kind.getKind() == InitializationKind::IK_Copy || >>>> Kind.isExplicitCast()) >>>> return ExprResult(Args.release()[0]); >>> >>> >>> ... reads to me as: >>> if (condition) >>> return statement; >>> <... some code that was never executed ...> >>> which I (at least hope that I did) transformed to: >>> assert(condition); >>> return statement; >>> <... delete code that was never executed ...> >>> which I would think is equivalent, given that the assert always holds >>> true. >>> >>> What am I missing? >>> >>> >>> The code that is currently never executed is still nonetheless correct, >>> and a minor tweak the any of the callers' handling of initialization >>> involving type-dependent expressions could make this code relevant again. If >>> we take your simplification, and one of those tweaks happen, we'll silently >>> transform ASTs into something semantically different and cause ourselves >>> some serious debugging pain. >>> >>> So if you want to simplify this code, I'm asking you to add appropriate >>> assertions to make sure that we catch the cases where this code will be >>> broken. In particular, the code itself is designed to handle NumArgs != 1, >>> but your simplification assumes NumArgs == 1 without checking. >>> >>> >>> Apparently, I'm just misreading patches right now. This change is fine; >>> sorry for the confusion. >>> >>> - Doug >>> >>> >> >> _______________________________________________ >> cfe-commits mailing list >> >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
