Skip to content

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Aug 14, 2025

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

@@ -23,6 +23,8 @@
* php zend_vm_gen.php
*/

#include <stdint.h>
Copy link
Member

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.)

Copy link
Member Author

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

Copy link
Member

@iluuu1994 iluuu1994 left a 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 */
Copy link
Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants