Skip to content

Kernel should have an AssertInitializer node #30914

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
johnniwinther opened this issue Sep 28, 2017 · 7 comments
Closed

Kernel should have an AssertInitializer node #30914

johnniwinther opened this issue Sep 28, 2017 · 7 comments
Assignees
Labels
customer-dart2js front-end-kernel legacy-area-front-end Legacy: Use area-dart-model instead. P2 A bug or feature request we're likely to work on

Comments

@johnniwinther
Copy link
Member

Kernel should have an AssertInitializer node. The current encoding misuses the LocalInitializer and makes the feature unnecessarily convoluted.

@peter-ahe-google
Copy link
Contributor

I disagree. Assert should be an expression.

@johnniwinther
Copy link
Member Author

So do you want an ExpressionInitializer as well or do you prefer to hide the assert in an LocalInitializer of an unused local of a weird valid (a value of type void is presume) ?.

@rakudrama
Copy link
Member

We need to omit the assert initializer when compile options dictate, so need to clearly match the entire thing, embedded side-effects and all.

We would like to avoid having to pattern-match inside some other initializer to attempt to identify the assert initializer. Perhaps an AssertInitializer contains an AssertExpression.

Whatever we do, we need something properly defined and described in kernel/lib/ast.dart in order to work on #30038 without having to go back for re-work as ideas change.

@peter-ahe-google
Copy link
Contributor

I'd use LocalInitializer. That's more flexible.

@rakudrama
Copy link
Member

@kmillikin As a first step, can we define in kernel/lib/ast.dart how an assert initializer will look?

Then the parser(s) can generate that tree, and the compilers can generate code for it.

The current form where dart2js sees a LocalInitializer containing a closure application is not suitable since it leaves residual code when assertions are not enabled.

@kmillikin
Copy link

As a first step:

AssertInitializer is an initializer that contains an AssertStatement. The representation is here: https://dart-review.googlesource.com/c/sdk/+/14760

Do you have any concerns with it?

@johnniwinther
Copy link
Member Author

It clearly marks the initializer so I'm ok with it.

@kmillikin kmillikin added legacy-area-front-end Legacy: Use area-dart-model instead. front-end-kernel and removed legacy-area-front-end Legacy: Use area-dart-model instead. labels Sep 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer-dart2js front-end-kernel legacy-area-front-end Legacy: Use area-dart-model instead. P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests

6 participants