Re: I wrote an extension: yajl-php
| From: | Nick Tolomiczenko | Date: | Wed, 09 Jan 2013 22:15:08 +0000 |
| Subject: | Re: I wrote an extension: yajl-php | ||
| References: | 1 2 | Groups: | php.pecl.dev |
| Request: | Send a blank email to pecl-dev+get-10203@lists.php.net to get a copy of this message | ||
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
Let me know if there is anything else I need to do.
Nick.
On 9 January 2013 06:19, Johannes Schlüter <johannes@schlueters.de> wrote:
> 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-phpunder
> > 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
>
>
>