GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Make zend_type into a struct with pointer + type_mask by nikic · Pull Request #4728 · php/php-src

Conversation

nikic commented Sep 20, 2019 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

This is the next step after #4727. It changes zend_type into a struct with a pointer and a type_mask. The type_mask contains the MAY_BE_* type union, flags to distinguish NAME/CE types (and other future type kinds), as well as space for extra flags. Extra flags are used by arg_info to replace pass_by_reference and is_variadic fields, which are encoded as part of zend_type now. This means that arg_info size does not increase.

We could also avoid increasing zend_property_info by storing flags in zend_type and making use of 32-bit padding in zend_type to store the property offset there. This is going to be a bit ugly though.

This should make the addition of union types a lot simpler, as we'll want to use both the pointer (either for a single class or a list of classes) there, and use the type_mask at the same time. However, I think it makes sense to also do this change independently, as all future type-system extensions are likely to need this (we have no more space for additional tag bits right now).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

nikic force-pushed the type-cleanup-2 branch 3 times, most recently from 612d7f6 to fbf46fa Compare

September 24, 2019 14:40

nikic changed the title ~~[WIP] Make zend_type into a struct with pointer + type_mask~~ Make zend_type into a struct with pointer + type_mask

Sep 24, 2019

nikic commented Sep 24, 2019

Copy link

Copy Markdown

Member Author

@dstogov This is ready for a look now.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nikic added 2 commits

September 25, 2019 11:31


          Make zend_type a 2-field struct

9f44caa


          Store pass_by_reference+is_variadic in type mask

de02cc3

nikic force-pushed the type-cleanup-2 branch from 5f95cda to de02cc3 Compare

September 25, 2019 09:32

dstogov reviewed

Sep 26, 2019

View reviewed changes

dstogov left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

In my opinion, this patch doesn't make any value, except for preparation for "union types".
It makes things more complex and messed + increases memory usage.

I won't agree with this patch alone.
It may fit into PHP together with "union types", however, I afraid they will bring much more mess.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Zend/zend.c

		new_arg_info[i].type =
		ZEND_TYPE_ENCODE_CLASS(
		name, ZEND_TYPE_ALLOW_NULL(arg_info[i].type));
		ZEND_TYPE_SET_PTR(new_arg_info[i].type, name);

dstogov Sep 26, 2019

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Would be better to name macro ZEND_TYPE_SET_CLASS_NAME()

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Zend/zend.c


		ZEND_ASSERT(prop_ce && prop_ce->type == ZEND_INTERNAL_CLASS);
		prop_info->type = ZEND_TYPE_ENCODE_CE(prop_ce, ZEND_TYPE_ALLOW_NULL(prop_info->type));
		prop_info->type = (zend_type) ZEND_TYPE_INIT_CE(prop_ce, ZEND_TYPE_ALLOW_NULL(prop_info->type), 0);

dstogov Sep 26, 2019

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'm not sure how modern C implements dynamic structure initializes.
I would recommend to use old style explicit macro initializers (like ZEND_LONG).

ZEND_TYPE_INIT_CE(rop_info->type, prop_ce, ZEND_TYPE_ALLOW_NULL(prop_info->type), 0);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Zend/zend_API.c

		ZEND_API int zend_declare_property_ex(zend_class_entry ce, zend_string name, zval property, int access_type, zend_string doc_comment) /* {{{ */
		{
		return zend_declare_typed_property(ce, name, property, access_type, doc_comment, ZEND_TYPE_ENCODE_NONE());
		return zend_declare_typed_property(ce, name, property, access_type, doc_comment, (zend_type) ZEND_TYPE_INIT_NONE(0));

dstogov Sep 26, 2019

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

"big" structure is passed by value. This is not efficient.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Zend/zend_API.h

		static const zend_internal_arg_info name[] = { \
		{ (const char*)(zend_uintptr_t)(required_num_args), ZEND_TYPE_ENCODE_CLASS_CONST(#class_name, allow_null), return_reference, 0 },
		{ (const char*)(zend_uintptr_t)(required_num_args), \
		ZEND_TYPE_INIT_CLASS_CONST(#class_name, allow_null, _ZEND_ARG_INFO_FLAGS(return_reference, 0)) },

dstogov Sep 26, 2019

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It's OK to use static initializers for constants.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Zend/zend_compile.c


		if (ast->kind == ZEND_AST_TYPE) {
		return ZEND_TYPE_ENCODE_CODE(ast->attr, allow_null);
		return (zend_type) ZEND_TYPE_INIT_CODE(ast->attr, allow_null, 0);

dstogov Sep 26, 2019

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Return "big" structure by value is not efficient.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Zend/zend_execute.c

		} else {
		zend_type type = ZEND_TYPE_WITHOUT_NULL(arg_info->type);
		switch (ZEND_TYPE_MASK(type)) {
		uint32_t type_mask = ZEND_TYPE_PURE_MASK_WITHOUT_NULL(arg_info->type);

dstogov Sep 26, 2019

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This PURE_MASK and FULL_MASK names are not intuitive.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Zend/zend_types.h

		*
		* ZEND_TYPE_IS_SET() - checks if type-hint exists
		* ZEND_TYPE_IS_MASK() - checks if type-hint refer to standard type
		* ZEND_TYPE_IS_ONLY_MASK() - checks if type-hint refer to standard type

dstogov Sep 26, 2019

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We don't have union types yet. I don't understand the meaning of "ONLY"?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

krakjoe added Feature Waiting on Author labels

Sep 30, 2019

nikic commented Oct 28, 2019

Copy link

Copy Markdown

Member Author

Part of #4838 now.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nikic closed this

Oct 28, 2019

Reviewers

dstogov

dstogov left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

Feature Waiting on Author

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

nikic commented Sep 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic commented Sep 24, 2019

Uh oh!

dstogov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic commented Oct 28, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nikic commented Sep 20, 2019 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.