Conversation
nikic
left a comment
nikic
left a comment
There was a problem hiding this comment.
Can you please also add a test involving these flags?
Added in d04a1d6 |
ast.c
Outdated
| #endif | ||
|
|
||
| #if PHP_VERSION_ID < 80400 | ||
| # define MODIFIER_PUBLIC_SET (1 << 10) |
There was a problem hiding this comment.
Seems like in PHP 7.3 ZEND_ACC_PRIVATE was 1024: https://github.com/php/php-src/blob/89dc78e0f0c1a4f27b889f232b109a3919ecf478/Zend/zend_compile.h#L214. But for all later versions it was updated to 4: https://github.com/php/php-src/blob/master/Zend/zend_compile.h#L220C9-L220C25.
Because of that introducing MODIFIER_PUBLIC_SET for 7.2./7.3 leads to failed tests. Is there a simple way to introduce modifiers from this PR only for PHP8.4+?
There was a problem hiding this comment.
@nikic seems like setting 0 for the new flags (<8.4) fixes this. Can you please give it a look? And run tests one more time? I`ve run 7.3 and 8.4 tests locally, and it seemed fine.
There was a problem hiding this comment.
I don't think it's a good idea to set these to zero. People can reasonably expect that the flags are all disjoint and non-zero. We should find values here that work (but don't have to match any specific PHP version).
There was a problem hiding this comment.
Sorry, I probably should be more clear in my initial comment. Please let me try doing better job at explaining my vision of the problem here:
- Until 7.3
1024value was used forZEND_ACC_PRIVATE, which is registered asMODIFIER_PRIVATE - In 7.4 value of
ZEND_ACC_PRIVATEwas changed from1024to4, and after 7.4 there were no any flags with1024value. - In 8.4 Asymmetric visibility v2 was introduced. And it introduced new
ZEND_ACC_PUBLIC_SETflag with1024value.
Because of that, we can't use 1024 value for MODIFIER_PUBLIC_SET for <8.4 versions. As it conflicts with ZEND_ACC_PRIVATE flag on <7.4.
Setting 0 value for MODIFIER_PUBLIC_SET flag for <8.4 seemed as simplest approach. As this flag (and other set modifiers) is not available in those PHP versions at all.
Seems like you are suggesting to use non-zero but not-conflicting (non-existing) flag values for set modifiers for versions where they were unavailable?
|
Dear @nikic could you please review the latest updates in this PR? Are there any other changes you would like to be included here? |
|
@nikic thank you very much for merging this! Can you please release a new version that includes updates from this PR? |
|
@SergiiDolgushev Sorry for the delay, I've released a new version now. |
@nikic thank you very much for releasing https://github.com/nikic/php-ast/releases/tag/v1.1.3 |
2 participants
Introduces new asymmetric visibility flags:
MODIFIER_PUBLIC_SET,MODIFIER_PROTECTED_SETandMODIFIER_PRIVATE_SET.