Skip to content

Add support for internal enums #7302

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
wants to merge 1 commit into from
Closed

Conversation

nikic
Copy link
Member

@nikic nikic commented Jul 23, 2021

This adds support for internal enums with the same basic approach as userland enums. Enum values are stored as CONSTANT_AST and objects created during constant updating at runtime. This means that we need to use mutable_data for internal enums.

This just adds basic support and APIs, it does not include the stubs integration from #7212.

@nikic nikic requested a review from dstogov July 23, 2021 14:02
@krakjoe
Copy link
Member

krakjoe commented Jul 24, 2021

I like this more than the persistent object approach.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are a bit scarce. Otherwise LGTM.

ce->enum_backing_type = type;
if (type != IS_UNDEF) {
ce->backed_enum_table = pemalloc(sizeof(HashTable), 1);
zend_hash_init(ce->backed_enum_table, 0, NULL, ZVAL_PTR_DTOR, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these will be mostly generated by stubs it would make sense to pass the right number of cases here right away.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think mostly is the point here. I don't see any reason to make the API more cumbersome than necessary.

Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any problems.

@nikic nikic closed this in a374230 Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants