[cfe-commits] [PATCH] Fix ParenListExpr with null type

Tue Jun 21 15:31:47 PDT 2011

On Tue, Jun 21, 2011 at 11:32 AM, Chandler Carruth <chandlerc at google.com>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 <klimek at google.com> 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 <dgregor at apple.com>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 <dgregor at apple.com>wrote:
>>>
>>>>
>>>> On Jun 15, 2011, at 10:11 AM, Manuel Klimek wrote:
>>>>
>>>> On Tue, Jun 14, 2011 at 8:21 PM, Douglas Gregor <dgregor at apple.com>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
>>
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110621/c4a35a99/attachment.html>