[PATCH] Closures and reflection improvements

17 years ago by Christian Seiler — view source

unread

Hi,

I'd like to add the following patch to reflection before the feature
freeze, because it does not change any current reflection behaviour but
adds quite a bit of good stuff for closures.

With the current closure implementation, __invoke always has the same
signature as the original lambda function, this makes passing parameters
per reference possible. But this does not show in current reflection:

$o = new ReflectionObject (function ($a) {});
$m = $o->getMethod ('__invoke');
var_dump ($m->getNumberOfParameters());

Currently, this does a lookup in the class function table. But the
signature of the method found in the function table has no parameters.
For the normal engine to have a different signature for a specific
closure, the get_method handler is used.

My patch does the following:

If a reflection method named '__invoke' is requested for an object
of the 'Closure' class (in any way possible: via constructor,
via ReflectionObject->getMethod, via ReflectionParameter
constructor, ...), the correct invoke signature is used by
reflection.
If the same method for the class 'Closure' (without an object)
is requested, the standard signautre (no parameters) will be used.
If you pass a callable object (closure or otherwise) as the only
parameter to the ReflectionMethod constructor, __invoke will be
used as default method. Example:
$m = new ReflectionMethod ($closure);
is identical to
$m = new ReflectionMethod ($closure, '__invoke');

For Post-5.3 we could even think of adding a new object handler for
retrieving methods for reflection generally (get_method will not work
correctly since the std_get_method handler does scope checking and
prints error messages) and thus allowing e.g. COM, Java or SOAP objects
to provide useful dynamic information for reflection. But to keep the
changes in 5.3 as non-invasive as possible, it is probably best to have
this single exception for closures.

You can find the patch here:

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-5.3.patch
http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-6.patch

The patch is fairly straight-forward and should be easily reviewed.

Here are some tests for this patch:

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-tests-5.3.tar.gz
http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-tests-6.tar.gz

I also found a segfault in all PHP versions in the ReflectionParameter
constructor. The above patches already fix that, but here's a patch that
fixes this for PHP 5.2:

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-segfault-fix-5.2.patch

For the segfault I also created following test case (works with 5.2, 5.3
and 6.0), NOT included in the above .tar.gzs.

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflectionParameter_invalidMethodInConstructor.phpt

Regards,
Christian

17 years ago by Christian Seiler — view source

unread

Hi,

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-5.3.patch

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-6.patch

The last CVS commit for apply_func_t and TSRMLS_CC conflicted with the
patches, I merged the conflicting line manually and reuploaded the
patches to the same location.

Regards,
Christian

17 years ago by Marcus Boerger — view source

unread

Hello Christian,

patch looks fine and should go in.

do you think ReflectionMethod::__construct implementation could be done
using parameterparsing 'f' rather than the spcial case 'o'? Unfortunately
we would still need the zs version unless we add another 'F' parameter
parsing that allows this two parameter case as well.

marcus

Friday, July 25, 2008, 5:41:49 PM, you wrote:

Hi,

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-5.3.patch

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-6.patch

The last CVS commit for apply_func_t and TSRMLS_CC conflicted with the
patches, I merged the conflicting line manually and reuploaded the
patches to the same location.

Regards,
Christian

Best regards,
Marcus

17 years ago by Christian Seiler — view source

unread

Hi Marcus,

patch looks fine and should go in.

Thanks.

do you think ReflectionMethod::__construct implementation could be done
using parameterparsing 'f' rather than the spcial case 'o'?

The Problem with 'f' is that it will accept every callback, even normal
functions, so that would kind of break the idea of having
ReflectionMethod different than ReflectionFunction.

So in my eyes, using 'o' for now is the sanest approach.

Regards,
Christian

17 years ago by Marcus Boerger — view source

unread

Hello Christian,

Friday, July 25, 2008, 9:01:13 PM, you wrote:

Hi Marcus,

patch looks fine and should go in.

Thanks.

do you think ReflectionMethod::__construct implementation could be done
using parameterparsing 'f' rather than the spcial case 'o'?

The Problem with 'f' is that it will accept every callback, even normal
functions, so that would kind of break the idea of having
ReflectionMethod different than ReflectionFunction.

So in my eyes, using 'o' for now is the sanest approach.

I see. So if we ever want to fix it we need 'm' for method then :-)

Best regards,
Marcus

17 years ago by Lukas Kahwe Smith — view source

unread

Hello Christian,

patch looks fine and should go in.

do you think ReflectionMethod::__construct implementation could be
done
using parameterparsing 'f' rather than the spcial case 'o'?
Unfortunately
we would still need the zs version unless we add another 'F' parameter
parsing that allows this two parameter case as well.

marcus

Friday, July 25, 2008, 5:41:49 PM, you wrote:

Hi,

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-5.3.patch

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-6.patch

if this isnt commited yet .. could someone handle this ASAP?

Lukas Kahwe Smith
mls@pooteeweet.org

16 years ago by Dmitry Stogov — view source

unread

Hi Marcus,

I have no objections against ZE part of the patch.
If you like ext/reflection part please commit the whole patch.

Thanks. Dmitry.

Christian Seiler wrote:

Hi,

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-5.3.patch

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-6.patch

The last CVS commit for apply_func_t and TSRMLS_CC conflicted with the
patches, I merged the conflicting line manually and reuploaded the
patches to the same location.

Regards,
Christian

16 years ago by johannes@php.net — view source

unread

Christian,

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-5.3.patch

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-6.patch

without applying and testing the patch:

zend_hash_apply_with_arguments(&ce->function_table TSRMLS_CC, (apply_func_args_t) _addmethod, 3, &ce, return_value, filter);

zend_hash_apply_with_arguments(&ce->function_table TSRMLS_CC, (apply_func_args_t) _addmethod, 3, &ce, return_value, filter, intern->obj);

that looks a bit strange, probably you want 4 instead of 3 now?

johannes

16 years ago by Marcus Boerger — view source

unread

Hello Christian,

I updated your patch for 5.3 due to recent changes. It works pretty fine
for me. Care to test again, especially with your new tests? Note that test
ext/reflection/tests/closures_001.phpt does not work for me and I have no
clue why. Well, besides that obviously the closure class no longer has an
entry for __closure, so it is more a question of how to know when to add
this. Also, it appears you do not have a cvs account, or did I overlook
something?

Modified patch attached, including the updated reflection tests.

Hello Johannes,

I addressed the thing you spotted.

best regards
marcus

Monday, August 4, 2008, 12:59:13 PM, you wrote:

Christian,

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-5.3.patch

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-6.patch

without applying and testing the patch:

  zend_hash_apply_with_arguments(&ce->function_table TSRMLS_CC,
(apply_func_args_t) _addmethod, 3, &ce, return_value, filter);
  zend_hash_apply_with_arguments(&ce->function_table TSRMLS_CC,
(apply_func_args_t) _addmethod, 3, &ce, return_value, filter, intern->obj);

that looks a bit strange, probably you want 4 instead of 3 now?

johannes

Best regards,
Marcus

16 years ago by Marcus Boerger — view source

unread

Hello Christian, Johannes,

Friday, August 8, 2008, 11:48:37 PM, you wrote:

Hello Christian,

I updated your patch for 5.3 due to recent changes. It works pretty fine
for me. Care to test again, especially with your new tests? Note that test
ext/reflection/tests/closures_001.phpt does not work for me and I have no
clue why. Well, besides that obviously the closure class no longer has an
entry for __closure, so it is more a question of how to know when to add
this. Also, it appears you do not have a cvs account, or did I overlook
something?

Modified patch attached, including the updated reflection tests.

Ok, it is actually easy to fix, we just check whether the thing is a
closure and add the method manually, done. New patch attached.

Johannes, this also adresses the switch from E_ERROR to E_RECOVERABLE_ERROR
for the closure handlers.

best regards
marcus

Monday, August 4, 2008, 12:59:13 PM, you wrote:

Christian,

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-5.3.patch

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-6.patch

without applying and testing the patch:

  zend_hash_apply_with_arguments(&ce->function_table TSRMLS_CC,
(apply_func_args_t) _addmethod, 3, &ce, return_value, filter);
  zend_hash_apply_with_arguments(&ce->function_table TSRMLS_CC,
(apply_func_args_t) _addmethod, 3, &ce, return_value, filter, intern->obj);

that looks a bit strange, probably you want 4 instead of 3 now?

johannes

Best regards,
Marcus

Best regards,
Marcus

16 years ago by Marcus Boerger — view source

unread

Hello Johannes, Christian, Felipe,

and here 5.3 and 6.0 versions that also have most memleaks fixed.

marcus

Saturday, August 9, 2008, 12:34:59 AM, you wrote:

Hello Christian, Johannes,

Friday, August 8, 2008, 11:48:37 PM, you wrote:

Hello Christian,

I updated your patch for 5.3 due to recent changes. It works pretty fine
for me. Care to test again, especially with your new tests? Note that test
ext/reflection/tests/closures_001.phpt does not work for me and I have no
clue why. Well, besides that obviously the closure class no longer has an
entry for __closure, so it is more a question of how to know when to add
this. Also, it appears you do not have a cvs account, or did I overlook
something?

Modified patch attached, including the updated reflection tests.

Ok, it is actually easy to fix, we just check whether the thing is a
closure and add the method manually, done. New patch attached.

Johannes, this also adresses the switch from E_ERROR to E_RECOVERABLE_ERROR
for the closure handlers.

best regards
marcus

Monday, August 4, 2008, 12:59:13 PM, you wrote:

Christian,

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-5.3.patch

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-6.patch

without applying and testing the patch:

  zend_hash_apply_with_arguments(&ce->function_table TSRMLS_CC,
(apply_func_args_t) _addmethod, 3, &ce, return_value, filter);
  zend_hash_apply_with_arguments(&ce->function_table TSRMLS_CC,
(apply_func_args_t) _addmethod, 3, &ce, return_value, filter, intern->obj);

that looks a bit strange, probably you want 4 instead of 3 now?

johannes

Best regards,
Marcus

Best regards,
Marcus

16 years ago by Dmitry Stogov — view source

unread

Hi Marcus,

I committed the ZE part of your patch with exception for

zend_error(E_ERROR, "Cannot destroy active lambda function");

Catching this error may cause memory corruption and crash.

Thanks. Dmitry.

Marcus Boerger wrote:

Hello Johannes, Christian, Felipe,

and here 5.3 and 6.0 versions that also have most memleaks fixed.

marcus

Saturday, August 9, 2008, 12:34:59 AM, you wrote:

Hello Christian, Johannes,

Friday, August 8, 2008, 11:48:37 PM, you wrote:

Hello Christian,

I updated your patch for 5.3 due to recent changes. It works pretty fine
for me. Care to test again, especially with your new tests? Note that test
ext/reflection/tests/closures_001.phpt does not work for me and I have no
clue why. Well, besides that obviously the closure class no longer has an
entry for __closure, so it is more a question of how to know when to add
this. Also, it appears you do not have a cvs account, or did I overlook
something?

Modified patch attached, including the updated reflection tests.

Ok, it is actually easy to fix, we just check whether the thing is a
closure and add the method manually, done. New patch attached.

Johannes, this also adresses the switch from E_ERROR to E_RECOVERABLE_ERROR
for the closure handlers.

best regards
marcus

Monday, August 4, 2008, 12:59:13 PM, you wrote:

Christian,

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-5.3.patch

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-6.patch

without applying and testing the patch:
  zend_hash_apply_with_arguments(&ce->function_table TSRMLS_CC,
(apply_func_args_t) _addmethod, 3, &ce, return_value, filter);
  zend_hash_apply_with_arguments(&ce->function_table TSRMLS_CC,
(apply_func_args_t) _addmethod, 3, &ce, return_value, filter, intern->obj);
that looks a bit strange, probably you want 4 instead of 3 now?

johannes

Best regards,
Marcus

Best regards,
Marcus

Best regards,
Marcus

16 years ago by Marcus Boerger — view source

unread

Hello Dmitry,

Monday, August 11, 2008, 10:51:44 AM, you wrote:

Hi Marcus,

I committed the ZE part of your patch with exception for

zend_error(E_ERROR, "Cannot destroy active lambda function");

Catching this error may cause memory corruption and crash.

Alright, didn't see that immediately.

Thanks. Dmitry.
Thanks marcus

Marcus Boerger wrote:
Hello Johannes, Christian, Felipe,

and here 5.3 and 6.0 versions that also have most memleaks fixed.

marcus

Saturday, August 9, 2008, 12:34:59 AM, you wrote:

Hello Christian, Johannes,

Friday, August 8, 2008, 11:48:37 PM, you wrote:

Hello Christian,

I updated your patch for 5.3 due to recent changes. It works pretty fine
for me. Care to test again, especially with your new tests? Note that test
ext/reflection/tests/closures_001.phpt does not work for me and I have no
clue why. Well, besides that obviously the closure class no longer has an
entry for __closure, so it is more a question of how to know when to add
this. Also, it appears you do not have a cvs account, or did I overlook
something?

Modified patch attached, including the updated reflection tests.

Ok, it is actually easy to fix, we just check whether the thing is a
closure and add the method manually, done. New patch attached.

Johannes, this also adresses the switch from E_ERROR to E_RECOVERABLE_ERROR
for the closure handlers.

best regards
marcus

Monday, August 4, 2008, 12:59:13 PM, you wrote:

Christian,

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-5.3.patch

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-6.patch

without applying and testing the patch:
  zend_hash_apply_with_arguments(&ce->function_table TSRMLS_CC,
(apply_func_args_t) _addmethod, 3, &ce, return_value, filter);
  zend_hash_apply_with_arguments(&ce->function_table TSRMLS_CC,
(apply_func_args_t) _addmethod, 3, &ce, return_value, filter, intern->obj);
that looks a bit strange, probably you want 4 instead of 3 now?

johannes

Best regards,
Marcus

Best regards,
Marcus

Best regards,
Marcus

Best regards,
Marcus

16 years ago by Marcus Boerger — view source

unread

Hello guys,

all submitted now. Thanks everyone for their help and especially
Christian for getting this started.

marcus

Monday, August 11, 2008, 12:23:57 PM, you wrote:

Hello Dmitry,

Monday, August 11, 2008, 10:51:44 AM, you wrote:

Hi Marcus,

I committed the ZE part of your patch with exception for

zend_error(E_ERROR, "Cannot destroy active lambda function");

Catching this error may cause memory corruption and crash.

Alright, didn't see that immediately.

Thanks. Dmitry.
Thanks marcus

Marcus Boerger wrote:
Hello Johannes, Christian, Felipe,

and here 5.3 and 6.0 versions that also have most memleaks fixed.

marcus

Saturday, August 9, 2008, 12:34:59 AM, you wrote:

Hello Christian, Johannes,

Friday, August 8, 2008, 11:48:37 PM, you wrote:

Hello Christian,

I updated your patch for 5.3 due to recent changes. It works pretty fine
for me. Care to test again, especially with your new tests? Note that test
ext/reflection/tests/closures_001.phpt does not work for me and I have no
clue why. Well, besides that obviously the closure class no longer has an
entry for __closure, so it is more a question of how to know when to add
this. Also, it appears you do not have a cvs account, or did I overlook
something?

Modified patch attached, including the updated reflection tests.

Ok, it is actually easy to fix, we just check whether the thing is a
closure and add the method manually, done. New patch attached.

Johannes, this also adresses the switch from E_ERROR to E_RECOVERABLE_ERROR
for the closure handlers.

best regards
marcus

Monday, August 4, 2008, 12:59:13 PM, you wrote:

Christian,

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-5.3.patch

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-closure-fixes-6.patch

without applying and testing the patch:
  zend_hash_apply_with_arguments(&ce->function_table TSRMLS_CC,
(apply_func_args_t) _addmethod, 3, &ce, return_value, filter);
  zend_hash_apply_with_arguments(&ce->function_table TSRMLS_CC,
(apply_func_args_t) _addmethod, 3, &ce, return_value, filter, intern->obj);
that looks a bit strange, probably you want 4 instead of 3 now?

johannes

Best regards,
Marcus

Best regards,
Marcus

Best regards,
Marcus

Best regards,
Marcus

Best regards,
Marcus

16 years ago by Christian Seiler — view source

unread

Hi,

all submitted now. Thanks everyone for their help and especially
Christian for getting this started.

Thanks for making my patch work after the modifications in
zend_closures.c. I'm sorry I couldn't do it myself but I was kind of
busy the last few days and the "simple fix" created memory leaks and I
didn't want to post a knowingly broken patch.

Anyway, there are still segfaults in the current version:

$foo = function ($a) {};
$o = new ReflectionMethod ($foo);
$p = $o->getParameters ();
unset ($o);
$m = $p[0]->getDeclaringFunction ();
Reflection::export ($m);

$foo = function ($a) {};
$p = new ReflectionParameter ($foo, 'a');
$m = $p->getDeclaringFunction ();
unset ($p);
Reflection::export ($m);

This is due to the fact that getParameters() / getDeclaringFunction()
don't copy the fptr and fptr->common.function_name pointers if it's a
closure. Here are patches for PHP 5.3 and HEAD that fix the problem:

http://www.christian-seiler.de/temp/php/2008-08-11-reflection/segfaults-5.3.patch
http://www.christian-seiler.de/temp/php/2008-08-11-reflection/segfaults-6.patch

Also, the following test that I already posted should be added to
PHP_5_2, PHP_5_3 and HEAD:

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflectionParameter_invalidMethodInConstructor.phpt

The test already works for PHP_5_3 (since the fix for that was included
in my original patch) but a segfault still occurs in PHP_5_2 and HEAD,
my patch that I already posted fixes that problem for 5.2:

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-segfault-fix-5.2.patch

My original patch also fixed the problem for HEAD, I readded that part
to the above segfault patch for HEAD.

Also, it appears you do not have a cvs account, or did I overlook
something?

No, I don't. Dmitry also mentioned that - should I request one?

Regards,
Christian

16 years ago by Marcus Boerger — view source

unread

Hello Christian,

Monday, August 11, 2008, 8:43:31 PM, you wrote:

Hi,

all submitted now. Thanks everyone for their help and especially
Christian for getting this started.

Thanks for making my patch work after the modifications in
zend_closures.c. I'm sorry I couldn't do it myself but I was kind of
busy the last few days and the "simple fix" created memory leaks and I
didn't want to post a knowingly broken patch.

Anyway, there are still segfaults in the current version:

$foo = function ($a) {};
$o = new ReflectionMethod ($foo);
$p = $o->getParameters ();
unset ($o);
$m = $p[0]->getDeclaringFunction ();
Reflection::export ($m);

$foo = function ($a) {};
$p = new ReflectionParameter ($foo, 'a');
$m = $p->getDeclaringFunction ();
unset ($p);
Reflection::export ($m);

This is due to the fact that getParameters() / getDeclaringFunction()
don't copy the fptr and fptr->common.function_name pointers if it's a
closure. Here are patches for PHP 5.3 and HEAD that fix the problem:

http://www.christian-seiler.de/temp/php/2008-08-11-reflection/segfaults-5.3.patch
http://www.christian-seiler.de/temp/php/2008-08-11-reflection/segfaults-6.patch

Also, the following test that I already posted should be added to
PHP_5_2, PHP_5_3 and HEAD:

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflectionParameter_invalidMethodInConstructor.phpt

The test already works for PHP_5_3 (since the fix for that was included
in my original patch) but a segfault still occurs in PHP_5_2 and HEAD,
my patch that I already posted fixes that problem for 5.2:

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-segfault-fix-5.2.patch

My original patch also fixed the problem for HEAD, I readded that part
to the above segfault patch for HEAD.

Also, it appears you do not have a cvs account, or did I overlook
something?

No, I don't. Dmitry also mentioned that - should I request one?

Yes, please do and the seg fix patch should have a function to do the
copying as it is not complete, so we can add to it as necessary.

Best regards,
Marcus

16 years ago by Marcus Boerger — view source

unread

Hello Christian,

Monday, August 11, 2008, 8:43:31 PM, you wrote:

Hi,

all submitted now. Thanks everyone for their help and especially
Christian for getting this started.

Thanks for making my patch work after the modifications in
zend_closures.c. I'm sorry I couldn't do it myself but I was kind of
busy the last few days and the "simple fix" created memory leaks and I
didn't want to post a knowingly broken patch.

Anyway, there are still segfaults in the current version:

$foo = function ($a) {};
$o = new ReflectionMethod ($foo);
$p = $o->getParameters ();
unset ($o);
$m = $p[0]->getDeclaringFunction ();
Reflection::export ($m);

$foo = function ($a) {};
$p = new ReflectionParameter ($foo, 'a');
$m = $p->getDeclaringFunction ();
unset ($p);
Reflection::export ($m);

This is due to the fact that getParameters() / getDeclaringFunction()
don't copy the fptr and fptr->common.function_name pointers if it's a
closure. Here are patches for PHP 5.3 and HEAD that fix the problem:

http://www.christian-seiler.de/temp/php/2008-08-11-reflection/segfaults-5.3.patch
http://www.christian-seiler.de/temp/php/2008-08-11-reflection/segfaults-6.patch

Also, the following test that I already posted should be added to
PHP_5_2, PHP_5_3 and HEAD:

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflectionParameter_invalidMethodInConstructor.phpt

The test already works for PHP_5_3 (since the fix for that was included
in my original patch) but a segfault still occurs in PHP_5_2 and HEAD,
my patch that I already posted fixes that problem for 5.2:

http://www.christian-seiler.de/temp/php/2008-07-24-reflection/reflection-segfault-fix-5.2.patch

My original patch also fixed the problem for HEAD, I readded that part
to the above segfault patch for HEAD.

Also, it appears you do not have a cvs account, or did I overlook
something?

No, I don't. Dmitry also mentioned that - should I request one?

Account approved and access granted. Please provide the function copying in
a function next to _free_function() and care for parameter info as well
(yeah sorry for the additional work). And then, happy submitting.

Also for your next patches (cvs di -N) allows to ship the new files in the
patch and (cvs di -Np) shows the context, which makes reading the patches
easier.

Best regards,
Marcus

16 years ago by Christian Seiler — view source

unread

Hi Marcus,

Account approved and access granted.

Thanks!

Please provide the function copying in a function next to _free_function()

I just committed that (and included some tests). I hope I didn't do
anything wrong in the process, if so, feel free to do so.

and care for parameter info as well

I assume with parameter info you mean arg_info? That isn't necessary and
actually harmful since the arg_info is not copied but simply referenced
via a C pointer in closures (zend_get_closure_invoke_method copies the
entire function structure, including the pointer to arg_info and only
changes some flags the function name). There is no need to copy or
destroy arg_info in reflection.

Also for your next patches (cvs di -N) allows to ship the new files in the
patch and (cvs di -Np) shows the context, which makes reading the patches
easier.

Ah, thanks, I added that to my .cvsrc.

Regards,
Christan

16 years ago by Marcus Boerger — view source

unread

Hello Christian,

Tuesday, August 12, 2008, 12:34:41 AM, you wrote:

Hi Marcus,

Account approved and access granted.

Thanks!

Please provide the function copying in a function next to _free_function()

I just committed that (and included some tests). I hope I didn't do
anything wrong in the process, if so, feel free to do so.

and care for parameter info as well

I assume with parameter info you mean arg_info? That isn't necessary and
actually harmful since the arg_info is not copied but simply referenced
via a C pointer in closures (zend_get_closure_invoke_method copies the
entire function structure, including the pointer to arg_info and only
changes some flags the function name). There is no need to copy or
destroy arg_info in reflection.

Oh, cool. I thought we'd have to do more stuff.

Also for your next patches (cvs di -N) allows to ship the new files in the
patch and (cvs di -Np) shows the context, which makes reading the patches
easier.

Ah, thanks, I added that to my .cvsrc.

Regards,
Christan

Best regards,
Marcus

16 years ago by Christian Seiler — view source

unread

Hi Dmitry, Hi Marcus,

I have no objections against ZE part of the patch.
If you like ext/reflection part please commit the whole patch.

Due to your cleanup wrt. handlers the reflection part will currently
(probably) segfault. I'll post an updated patch for this tomorrow.

Regards,
Christian