This is an archive of the discontinued LLVM Phabricator instance.

Fix PR48889: assertion failure for void() in flattened ternary expression
AcceptedPublic

Authored by aaronpuchert on Mar 28 2021, 5:32 AM.

Details

Reviewers
rsmith
Summary

We treat the expression void() as CXXScalarValueInitExpr for historic
reasons probably, but void isn't a scalar type. The current standard
considers this a functional cast expression, where void is explicitly
exempted from initialization.

Diff Detail

Event Timeline

aaronpuchert requested review of this revision.Mar 28 2021, 5:32 AM
aaronpuchert created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2021, 5:32 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Replace E->getType() argument by T.

rsmith accepted this revision.Mar 28 2021, 11:32 AM
rsmith added inline comments.
clang/lib/CodeGen/CGExprScalar.cpp
474–475

Would it make sense to put this check in EmitNullValue instead? That would also cover an InitListExpr for void{} (which we currently appear to incorrectly reject), and an ImplicitValueInit for type void (which I don't think can be created legitimately at the moment, but that could just be a failure of imagination on my part).

This revision is now accepted and ready to land.Mar 28 2021, 11:32 AM
aaronpuchert added inline comments.Mar 28 2021, 3:01 PM
clang/lib/CodeGen/CGExprScalar.cpp
474–475

You're right, we do reject void{}. Perhaps something I should address first, so that we can have tests for both cases.

I was considering putting this in EmitNullValue, but then I read through the standard and it seems void isn't value/zero-initializable. Then I found [expr.type.conv] where it says void() is allowed and “performs no initialization.” So if we stick close to the standard it seems we should catch the special case here, but in EmitNullValue it would indeed cover both void() and void{}.

Regarding void{}: you might know that InitListExprs have void type initially, and there is an assertion that it doesn't have void type after Sema ran over it. I'd have to remove that assertion, unless we give InitListExprs a different initial type. Maybe you can comment on D98664 what you think about this, where I'm trying to bring ParenListExpr in line with InitListExpr regarding the initial type.

aaronpuchert marked an inline comment as done.

Also fix code generation for void{}.

shafik added a subscriber: shafik.Sep 21 2023, 12:46 PM

This is accepted but it looks like we need the previous void{} fix in order to land this. It would be nice to get both in before phab is not usable anymore.

Herald added a project: Restricted Project. · View Herald TranscriptSep 21 2023, 12:46 PM