-
Notifications
You must be signed in to change notification settings - Fork 8k
Add ZEND_VM_KIND constant #19574
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
Add ZEND_VM_KIND constant #19574
Conversation
|
I think it would be fine to just expose |
While I tend to agree with this because it's a bit of a niche use case. The current proposal does make for a nicer API for the end user given we didn't go as far as to make it an enumeration. TL;DR I think the current PR is just fine for what we are looking to achieve |
I disagree. The other constants are not really properly discoverable either and with the "random integer value" they are just fancy strings. So we might as well use a string directly. |
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.
No RM objections for this to go into 8.5 if desired.
No technical review performed.
|
I am happy to expose it as a string, but then we can't use the stub like this, as the strings aren't part of PHP yet. Will have a look at this again then. |
e25e800 to
cec179b
Compare
cec179b to
b28de03
Compare
fb4e2ec to
439a3ab
Compare
e210d60 to
9303bee
Compare
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.
Unfortunately this breaks zend_vm_gen.php as the script defines ZEND_VM_KIND too. I suggest renaming ZEND_VM_KIND to ZEND_VM_MAIN_KIND or ZEND_VM_GEN_KIND in that script.
Looks good to me otherwise!
…for internal usage
Per request/suggestion from @sebastianbergmann