Re: I wrote an extension: yajl-php

Hi, On Tue, 2013-01-08 at 13:47 -0500, Nick Tolomiczenko wrote: > I recently came across the need to parse JSON in php with very low memory > consumption. The unfortunately the json_decode() function, currently in > php, was too memory aggressive when the JSON was huge. I needed a SAX-like > incremental parser in php. The C based yajl library, written by lloyd, was > exactly what I needed; but I needed it in php. So I wrote the yajl-php > extension which is available at: > https://github.com/neikos/yajl-php under > the PHP license version 3.01 That's a nice thing! I had a quick run over the code, a few small observations: * Code layout differs sometimes a bit, would be nice to unify indention (PHP uses single tabs) * In at least on error message (php_error_docref) you're ending with a full stop '.' this would result in an error like "This is a sentence. in file.php line 3", not beautiful. * You're using C99 Designated Initializers. PHP uses C89/C90 . Using C99 limits portability as some compilers, most notably MS Visual Studio, have limited support. I haven't checked yajl, if they have the same portability restriction it's fine. See also https://wiki.php.net/internals/review_comments#don_t_use_c99_for_portability_reasons * Including #include <yajl/yajl_parse.h> from php_yajl.h isn't good. I suggest moving that include and the yajl_parser typedef to the .c file. See https://wiki.php.net/internals/review_comments#php_extnameh_should_be_minimal * Please also see this note on empty RINIT/RSHUTDOWN: https://wiki.php.net/internals/review_comments#empty_rinitrshutdown_functions_should_be_removed * You are using resources, that is fine while we tend to use objects instead. Looks like a long list, but all small things in fact. johannes