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

Manuel Klimek klimek at google.com
Thu Jun 16 23:47:09 PDT 2011


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?

Cheers,
/Manuel

Apart from that I realized that I hadn't run it over all our code in debug
> mode - I now started this, and promptly ran into an assert - it's not in the
> dead code, though, but in the code that actually adds the type
>
> assert.h assertion failed at
> third_party/llvm/trunk/tools/clang/include/clang/AST/Expr.h:83 in void
> clang::Expr::setType(clang::QualType): (t.isNull() || !t->isReferenceType())
> && "Expressions can't have reference type"
>
> So apparently sometimes the VDecl->getType() is a reference type...
> What's the correct solution here?
>
>
> VDecl->getType().getNonReferenceType().
>
> - Doug
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110616/83ba9d28/attachment.html>


More information about the cfe-commits mailing list