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

[Douglas Gregor]

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.

  - Doug

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110617/d4ad662e/attachment.html>