php_reflection.c: make a bunch of pointers const
#15927
Conversation
Co-authored-by: Niels Dossche <[email protected]>
|
Failures appear to be from things timing out, should be unrelated |
Co-authored-by: Ilija Tovilo <[email protected]>
| static void _extension_string(smart_str *str, zend_module_entry *module, char *indent); | ||
| static void _zend_extension_string(smart_str *str, zend_extension *extension, char *indent); | ||
| static void _const_string(smart_str *str, const char *name, zval *value, const char *indent); | ||
| static void _function_string(smart_str *str, zend_function *fptr, zend_class_entry *scope, const char* indent); |
There was a problem hiding this comment.
One more question: Based on what rules did you add const? It seems a bit arbitrary to add it for zend_extension, but not zend_function, zval, etc. I don't object about these changes in general, but I'm not sure it makes sense to touch all functions just to add them, at least not unless we can show that there's some code-generation benefit from it. In this isolated context, the changes look fine.
There was a problem hiding this comment.
I added as many as I could
E.g. for _const_string, the zval *value gets passed to zval_get_tmp_string() which requires a non-const zval, since (digging deeper into the call stack) for IS_STRING values the reference count is increased, which needs non-constant inputs
For _function_string, the function pointer is passed to _function_parameter_string() and from there to _parameter_string() which needs to cast it to (zend_op_array*), so a non-constant input is needed
If this (and a few other smaller test patches I was going to send, the next being for zend_enum.c) were accepted, I would then file a task/start a discussion for doing this all over the place. E.g., if zend_string_dup() gets changed to get a constant pointer, then anything that calls (and doesn't need a non-constant pointer for anything else), and so on, bubbling up.
|
Thank you @DanielEScherzer! |
Thanks for the review - I sent the zend_enum.c patch as #15932 |
No description provided.