-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Zend: Convert certain fields to uint32_t #19479
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
base: master
Are you sure you want to change the base?
Conversation
@@ -23,6 +23,8 @@ | |||
* php zend_vm_gen.php | |||
*/ | |||
|
|||
#include <stdint.h> |
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 change is ignored (it doesn't end up in zend_vm_execute.h
). I would recommend to add this include to zend_vm_execute.skl
instead. (stdint.h
is probably already included where zend_vm_execute.h
is included, but this is still useful when looking at this file independently.)
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.
Oh I didn't even realize this header got added, will add to the skeleton
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.
One small thing to consider is that unsigned can sometimes lead to slightly worse code because overflows aren't UB. But the Valgrind instruction count does not indicate this is a problem.
int last_var; /* number of CV variables */ | ||
uint32_t last; /* number of opcodes */ | ||
uint32_t cache_size; /* number of run_time_cache_slots * sizeof(void*) */ | ||
int last_var; /* number of CV variables */ |
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.
Why is last_var
excluded?
These basically always assign to
uint32_t
or be used as such anyway, so might as well make them this type properly so we don't need to think about C's implicit sign conversion rules