Page MenuHomePhabricator

Fix crash on dumping AST containing constant initializer with ParenListExpr
AcceptedPublic

Authored by aaronpuchert on Mar 15 2021, 3:35 PM.

Details

Summary

The crash was introduced by D83183 (f63e3ea558bb) as far as I can tell.
The problem is that ParenListExprs have a <NULL TYPE> which the
expression evaluation, specifically Expr::EvaluateAsInitializer,
couldn't deal with. One solution would have been to make the evaluation
logic capable of dealing with NULL TYPE, but I rather decided to
harmonize ParenListExprs with InitListExprs, which always have a void
type by default.

Actually I think that the dependent type would not be a bad fit here:
although the type of the contained expression might not be dependent,
the type of the expression itself might be. But this seems like a bigger
change, the assumption that InitListExprs have void type seems to be
baked into quite a few places.

Diff Detail

Event Timeline

aaronpuchert created this revision.Mar 15 2021, 3:35 PM
aaronpuchert requested review of this revision.Mar 15 2021, 3:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2021, 3:35 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaronpuchert added inline comments.Mar 15 2021, 3:37 PM
clang/test/AST/ast-dump-templates.cpp
71–73

This is what's currently crashing.

As far as the changes go, these seem reasonable to me, though I find it a bit odd that these are expressions without a type whereas a ParenExpr has a type of the underlying parenthesized expression. e.g.,

int x(0); // (0) has void type
int y = (0); // (0) has int type

I think my natural assumption is that the init expression would have the type of the thing that's being initialized.

Despite that, I think the changes LG, but I'd like to hear from @rsmith.

As far as the changes go, these seem reasonable to me, though I find it a bit odd that these are expressions without a type whereas a ParenExpr has a type of the underlying parenthesized expression. e.g.,

int x(0); // (0) has void type
int y = (0); // (0) has int type

I think my natural assumption is that the init expression would have the type of the thing that's being initialized.

In this case the int type will be assigned when the initialization is carried out in Sema. The void will only remain if we are in a template and the variable being initialized has a dependent type, so we can't carry out the initialization. Also if there are errors. It's similar to int y{0}; in that regard.

If we don't know the type this syntax could be a constructor call, conversion operator call, but also regular value initialization. So we can't use CXXUnresolvedConstructExpr or something like that. That's my understanding.

Despite that, I think the changes LG, but I'd like to hear from @rsmith.

So would I, also about whether to use the dependent type instead.

aaron.ballman accepted this revision.Mar 16 2021, 6:09 AM

As far as the changes go, these seem reasonable to me, though I find it a bit odd that these are expressions without a type whereas a ParenExpr has a type of the underlying parenthesized expression. e.g.,

int x(0); // (0) has void type
int y = (0); // (0) has int type

I think my natural assumption is that the init expression would have the type of the thing that's being initialized.

In this case the int type will be assigned when the initialization is carried out in Sema. The void will only remain if we are in a template and the variable being initialized has a dependent type, so we can't carry out the initialization. Also if there are errors. It's similar to int y{0}; in that regard.

If we don't know the type this syntax could be a constructor call, conversion operator call, but also regular value initialization. So we can't use CXXUnresolvedConstructExpr or something like that. That's my understanding.

Ah, thank you for the explanation, that makes sense.

Despite that, I think the changes LG, but I'd like to hear from @rsmith.

So would I, also about whether to use the dependent type instead.

I'm giving my LG, but you should wait for a few days before landing to see if @rsmith has concerns.

This revision is now accepted and ready to land.Mar 16 2021, 6:09 AM