Re: [PATCH] readline extension bug fixes and enhancements
Hello
On Mar 20, 2012, at 9:43 PM, Johannes Schlüter wrote:
>
> Have you read Nikita's and my comments on your mail from last week?
>
I just did. There was a problem with my mailing list subscription (wasn't confirmed) so I
thought it didn't go through. But the good thing is that I had ported it to 5.4 and found the
memory corruption bug.
Here are my answers to your comments:
On Mar 14, 2012, at 10:28 PM, Johannes Schlüter wrote:
> A) for licensing reasons we should try to keep as few readline only things as possible in there
> (gpl vs. php license)
I understand the issue of licensing but the extension already has readline-specific functionality so
I thought why not. However if this is going to be a problem then I can take out all
readline-sepcific functions into a separate module altogether and keep this one for editline
functions only.
> B) thread safty isn't an issue for readline but you still should do the init and deinit in
> rinit/rshutdown not minit/mshutdown, probably also do only bind_key_functions =NULL in rinit and
> init the hashtable on demand later.
Roger that.
> C) don't compare the r result of zend_hash_find and others against -1 or such but use
> SUCCESS/FAILURE
Roger that.
> D) i don't really like new features in 5.3 anymore at this stage
This last patch is ported to 5.4.
> E) please log a bug and attach the patch so we can track this
Roger that. After I clear out which direction to take based on your answers.
And as for Nikita's comments:
On Mar 14, 2012, at 10:47 PM, Nikita Popov wrote:
> May I ask where you got this pattern for copying zvals from? Is this
> kind of code shown in some tutorial / manual / etc? Such code is used
> all other the place, instead of the MAKE_COPY_ZVAL macro and I wonder
> why. Doing manual copying has a good chance of leaking memory. Just
> from glancing I'd say that your code will leak if arg has a refcount >
> 1 (but I could be wrong) as it does not PINIT the zval after it was
> copied.
Two points here:
A) The new patch does not use this at all because it really isn't needed
B) I got this from the same readline.c file lines 521 and 574 and 579. But I will change it
accordingly
Thread (4 messages)