Hi Johannes, Your comments are awesome!
I have more code to read and, most probably, will have some questions before I proceed. Nick. On 10 January 2013 08:14, Johannes Schlüter <[email protected]> wrote: > On Wed, 2013-01-09 at 17:15 -0500, Nick Tolomiczenko wrote: > > Hi Johannes, > > > > I made all changes except the last (optional) one regarding the use of > > objects instead of resources. I will do that in a subsequent release if > > that's okay. I pushed the changes to > https://github.com/neikos/yajl-php.git > > I wouldn't change after a release - either stick with the resource-based > approach or go to an OO interface. I for one could imagine having an > abstract base class the user can extend to create his parser ... on the > other hand the current approach allows things like > > yajl_set_string_handler($p, function() { echo "yay, a string"; }); > > which also is nice ... so that's a design decision you have to take :-) > > > > Ah and another comment: You can simplify the code a tiny bit by using > the "f" modifier for zend_parse_parameters, this gives you the function > information and function pointer. There is one caveat, though: When > storing it you have to mind the refcount of a potential object. Stealing > my code from https://github.com/johannes/php-test-helpers this looks > something like this: > > typedef struct { > zend_fcall_info fci; > zend_fcall_info_cache fcc; > } handler_t; > > PHP_FUNCTION(foo) { > zend_fcall_info fci; > zend_fcall_info_cache fcc; > > if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "f", &fci, > &fcc) == FAILURE) { > return; > } > > handler->fci = fci; > handler->fcc = fcc; > Z_ADDREF_P(handler->fci.function_name); > #if PHP_VERSION_ID >= 50300 > if (handler->fci.object_ptr) { > Z_ADDREF_P(handler->fci.object_ptr); > } > #endif > } > > void some_func() { > zend_fcall_info_argn(&handler->fci TSRMLS_CC, 1, &arg); > zend_fcall_info_call(&handler->fci, &handler->fcc, &retval, NULL > TSRMLS_CC); > zend_fcall_info_args_clear(&handler->fci, 1); > } > > void free_handler() { > if (handler->fci.function_name) { > zval_ptr_dtor(&handler->fci.function_name); > handler->fci.function_name = NULL; > } > #if PHP_VERSION_ID >= 50300 > if (handler->fci.object_ptr) { > zval_ptr_dtor(&handler->fci.object_ptr); > handler->fci.object_ptr = NULL; > } > #endif > } > > Benefit is that the user gets a more consistent error message > (zend_parse_parameters will emit one, being the same for all extensions > using this) and by using the stored fcc one safes some lookup time > afterwards making it a tiny bit faster. Probably you won't need 5.2 > compatibility, then the #ifdef's can go. (also see > php-src/README.PARAMETER_PARSING) > > johannes > >
