Conversation
The zval change impacted this code throughout.
|
Thanks so much for your thorough review. I realize the initial code in the PR did not pass all the tests under a version of PHP compiled without debug enabled. I'll see if fixing the issues you mention helps otherwise I might ask for more help. Any further comments will be appreciated. |
|
OK, all tests now pass both with a php binary with debug enabled or disabled. Together with the changes you identified, I added the destructor call for the buffer zval in The code now compiles without warnings, which I should have heed before as they pointed to the |
| size_t length; | ||
|
|
||
| /* For Iterator methods */ | ||
| size_t current_offset; |
There was a problem hiding this comment.
Orthogonal to your changes, but BufferView should really be implementing IteratorAggregate, not Iterator. It doesn't make a lot of sense that the view itself has a current position, as opposed to an iterator over the view having a position.
This is easy in PHP 8 because it's possible to automatically create an InternalIterator object from the buffer_view_iterator, but in earlier versions it requires actually implementing the iterator class.
There was a problem hiding this comment.
After this, I want to do a migration to PHP 8, maybe leave that change for then? Otherwise I can give this a try now, just let me know.
Something else we need for Rubix is for Buffer to implement the Countable interface, that's also next for me but I hope the current migration is solid before adding changes.
| } | ||
| } | ||
| if(Z_TYPE_P(zbuf) != IS_STRING) { | ||
| zend_throw_exception(NULL, "Could not unserialize buffer: not a string", 0); |
There was a problem hiding this comment.
I think we're leaking zbuf here? (Not sure about previous branch, would have to check what php_var_unserialize does on error...)
Same also for some cases below.
There was a problem hiding this comment.
Good point.
Looking at the code in ext/standard/var_unserializer.c, this zbuf is a pointer inside the var hash which in turn has its destructors called in the var_destroy function (chained from the PHP_VAR_UNSERIALIZE_DESTROY on the exit). So it should be fine.
Co-authored-by: Nikita Popov <github@npopov.com>
Co-authored-by: Nikita Popov <github@npopov.com>
Co-authored-by: Nikita Popov <github@npopov.com>
Co-authored-by: Nikita Popov <github@npopov.com>
|
I added the latest changes. Slowly gaining on my understanding of PHP internals. Thanks so much for your guidance. |
|
Thanks! It would probably be good to add some basic Github Actions setup to check that things build with versions which are intended to be supported. Always a bit tricky with differences across versions. |
2 participants
The zval change impacted this code throughout, this close to a rewrite.
I know this is very old code, but I'm looking at whether it can help speed up the tensor library in Rubix/ML. A contiguous C array of floats is exactly what we need to interface to the linear algebra C libraries.