Make zend_type into a struct with pointer + type_mask #4728
Conversation
nikic
force-pushed
the
type-cleanup-2
branch
3 times, most recently
from
612d7f6 to
fbf46fa
Compare
nikic
changed the title
|
@dstogov This is ready for a look now. |
nikic
force-pushed
the
type-cleanup-2
branch
from
5f95cda to
de02cc3
Compare
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
Would be better to name macro ZEND_TYPE_SET_CLASS_NAME()
| @@ -968,7 +965,7 @@ static void zend_resolve_property_types(void) /* {{{ */ | |||
| zend_class_entry *prop_ce = zend_hash_find_ptr(CG(class_table), lc_type_name); | |||
|
|
|||
| 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); | |||
There was a problem hiding this comment.
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);
| @@ -3719,7 +3708,7 @@ ZEND_API int zend_try_assign_typed_ref_zval_ex(zend_reference *ref, zval *zv, ze | |||
|
|
|||
| 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)); | |||
There was a problem hiding this comment.
"big" structure is passed by value. This is not efficient.
|
|
||
| #define ZEND_BEGIN_ARG_WITH_RETURN_OBJ_INFO_EX(name, return_reference, required_num_args, class_name, allow_null) \ | ||
| 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)) }, |
There was a problem hiding this comment.
It's OK to use static initializers for constants.
| @@ -5312,7 +5311,7 @@ static zend_type zend_compile_typename(zend_ast *ast, zend_bool force_allow_null | |||
| } | |||
|
|
|||
| 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); | |||
There was a problem hiding this comment.
Return "big" structure by value is not efficient.
| @@ -711,8 +711,8 @@ static ZEND_COLD void zend_verify_type_error_common( | |||
| *need_kind = ZSTR_VAL(ZEND_TYPE_NAME(arg_info->type)); | |||
| } | |||
| } 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); | |||
There was a problem hiding this comment.
This PURE_MASK and FULL_MASK names are not intuitive.
| @@ -106,87 +106,106 @@ typedef void (*copy_ctor_func_t)(zval *pElement); | |||
| * It shouldn't be used directly. Only through ZEND_TYPE_* macros. | |||
| * | |||
| * 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 | |||
There was a problem hiding this comment.
We don't have union types yet. I don't understand the meaning of "ONLY"?
|
Part of #4838 now. |
This is the next step after #4727. It changes
zend_typeinto 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).