Fix GH-9967 Add support for generating custom function, class const, and property attributes in stubs #11978
Conversation
kocsismate
requested review from
dstogov,
iluuu1994,
bukka and
nielsdos
as code owners
kocsismate
force-pushed
the
stub-general-attributes
branch
from
b7ba089 to
d022d84
Compare
Zend/zend_attributes_arginfo.h
Outdated
| zval attribute_Attribute_class_Attribute_arg0; | ||
| ZVAL_LONG(&attribute_Attribute_class_Attribute_arg0, ZEND_ATTRIBUTE_TARGET_CLASS); | ||
| ZVAL_COPY_VALUE(&attribute_Attribute_class_Attribute->args[0].value, &attribute_Attribute_class_Attribute_arg0); | ||
| zend_string *attribute_name_Attribute_class_Attribute_1 = zend_string_init_interned("Attribute", sizeof("Attribute") - 1, 1); |
There was a problem hiding this comment.
Duplicate attributes are rare. I propose to not add _1 if there are no duplicates.
If there are, 0-based indexing looks more natural to me, as everything in PHP is 0-based indexed.
There was a problem hiding this comment.
Conditionally adding/not adding the attributes would complicate the very complicated code even more. Since these files are not intended to be human readable (the git attribute change was pushes inadvertently, which I'll remove right now), I don't see much reason about using either index-less names, or 0-based indices. But other reviewers can tell their preference.
There was a problem hiding this comment.
Conditionally adding/not adding the attributes would complicate the very complicated code even more.
Agree, probably not worth the trouble...
Since these files are not intended to be human readable
That's not their primary goal indeed, but please keep in mind that given the scarcity of internals documentation, the output of gen_stubs is for extension developers the only available documentation on how to do certain things. So keeping some level of readability is a good idea. Therefore I tend to agree that 0-based indexing is more intuitive since it's used almost everywhere.
There was a problem hiding this comment.
At last, I went with 0-based indices as I don't really care which style is used.
kocsismate
force-pushed
the
stub-general-attributes
branch
2 times, most recently
from
e79e148 to
9c7dd19
Compare
|
@kocsismate You can view the diff of generated files with |
kocsismate
force-pushed
the
stub-general-attributes
branch
from
9c7dd19 to
fa7464b
Compare
They are now used in arginfo files.
kocsismate
force-pushed
the
stub-general-attributes
branch
from
fa7464b to
bfe35a0
Compare
…t, and property attributes in stubs
kocsismate
force-pushed
the
stub-general-attributes
branch
from
60e521d to
903863d
Compare
| #else | ||
| /* FIXME: We could add (zend_type) here at some point but this breaks in MSVC because | ||
| * (zend_type)(zend_type){} is no longer considered constant. */ | ||
| # define _ZEND_TYPE_PREFIX |
There was a problem hiding this comment.
This broke something with C++ extensions with clang on MacOS, I get errors like this on 8.3.0RC1:
/Users/runner/work/PHP-Binaries/PHP-Binaries/install_data/subdir/php/ext/morton/morton.cpp:45:1: error: expected '(' for function-style cast or type construction
ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_morton2d_encode, 0, 2, IS_LONG, 0)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/runner/work/PHP-Binaries/PHP-Binaries/install_data/subdir/php/Zend/zend_API.h:202:2: note: expanded from macro 'ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX'
ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX2(name, return_reference, required_num_args, type, allow_null, 0)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/runner/work/PHP-Binaries/PHP-Binaries/install_data/subdir/php/Zend/zend_API.h:199:50: note: expanded from macro 'ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX2'
{ (const char*)(uintptr_t)(required_num_args), ZEND_TYPE_INIT_CODE(type, allow_null, _ZEND_ARG_INFO_FLAGS(return_reference, 0, is_tentative_return_type)), NULL },
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/runner/work/PHP-Binaries/PHP-Binaries/install_data/subdir/php/Zend/zend_types.h:287:2: note: expanded from macro 'ZEND_TYPE_INIT_CODE'
ZEND_TYPE_INIT_MASK(((code) == _IS_BOOL ? MAY_BE_BOOL : ( (code) == IS_ITERABLE ? _ZEND_TYPE_ITERABLE_BIT : ((code) == IS_MIXED ? MAY_BE_ANY : (1 << (code))))) \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/runner/work/PHP-Binaries/PHP-Binaries/install_data/subdir/php/Zend/zend_types.h:284:20: note: expanded from macro 'ZEND_TYPE_INIT_MASK'
_ZEND_TYPE_PREFIX { NULL, (_type_mask) }
~~~~~~~~~~~~~~~~~ ^
/Users/runner/work/PHP-Binaries/PHP-Binaries/install_data/subdir/php/ext/morton/morton.cpp:46:2: error: expected '(' for function-style cast or type construction
ZEND_ARG_TYPE_INFO(0, x, IS_LONG, 0)
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/runner/work/PHP-Binaries/PHP-Binaries/install_data/subdir/php/Zend/zend_API.h:135:11: note: expanded from macro 'ZEND_ARG_TYPE_INFO'
{ #name, ZEND_TYPE_INIT_CODE(type_hint, allow_null, _ZEND_ARG_INFO_FLAGS(pass_by_ref, 0, 0)), NULL },
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/runner/work/PHP-Binaries/PHP-Binaries/install_data/subdir/php/Zend/zend_types.h:287:2: note: expanded from macro 'ZEND_TYPE_INIT_CODE'
ZEND_TYPE_INIT_MASK(((code) == _IS_BOOL ? MAY_BE_BOOL : ( (code) == IS_ITERABLE ? _ZEND_TYPE_ITERABLE_BIT : ((code) == IS_MIXED ? MAY_BE_ANY : (1 << (code))))) \
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/runner/work/PHP-Binaries/PHP-Binaries/install_data/subdir/php/Zend/zend_types.h:284:20: note: expanded from macro 'ZEND_TYPE_INIT_MASK'
_ZEND_TYPE_PREFIX { NULL, (_type_mask) }
~~~~~~~~~~~~~~~~~ ^
Extension source for reference: https://github.com/pmmp/ext-morton/blob/2532e9f09d3bc6f6975d78b6632086b894dcef75/morton.cpp#L45
There was a problem hiding this comment.
(Commented on the wrong line)
2nd take after the revert of #10170