Conversation
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
I'm not sure this is a good idea, it's kinda neat that you can store the data in the embedded zval. Having 8 extra bytes (for the type info) in the structure is not critical. Trading this for an extra allocation doesn't seem to be good for performance either, not that any of this is likely performance critical anyway
| } | ||
| case REF_TYPE_REFERENCE: | ||
| zval_ptr_dtor(intern->ptr); | ||
| efree(intern->ptr); |
Arguably it's confusing that a zval field called "obj" is being used to store a reference, especially when the 71 other usages are all about dealing with closures. This feature is very targeted as explained from the RFC: https://wiki.php.net/rfc/reference_reflection. However there is no explanation about the odd reusing of the zval on the associated PR. |
You could use a union then to make the code clearer |
Rather than re-using the reflection_object.obj field that usually holds a zend_object* to a Closure. Precondition for a future refactoring to convert the reflection_object.obj from zval to zend_object*
0a6bf00 to
85f79d7
Compare
| //if (Z_TYPE_P(ref) != IS_REFERENCE) { | ||
| // zend_throw_exception(reflection_exception_ptr, "Corrupted ReflectionReference object", 0); | ||
| // RETURN_THROWS(); | ||
| //} |
There was a problem hiding this comment.
We didn't have any tests for this case and I don't really see how this could happen.
Running with the union idea to instead store a zend_reference* seems to work. Which also mean when converting the closure storage from the zval to a zend_object* we still gain the struct size reduction. :) |
2 participants
Rather than re-using the reflection_object.obj field that usually holds a zend_object* to a Closure.
Precondition for a future refactoring to convert the reflection_object.obj from zval to zend_object*