Re: Re: Always set return_value_ptr?

From: Date: Fri, 30 Aug 2013 00:30:06 +0000
Subject: Re: Re: Always set return_value_ptr?
References: 1 2  Groups: php.internals 
Request: Send a blank email to [email protected] to get a copy of this message 
On 27/08/13 10:40, Nikita Popov wrote:

On Sat, Aug 3, 2013 at 8:16 PM, Nikita Popov <[email protected]> wrote:


Hi internals!

Is there any particular reason why we only pass return_value_ptr to
internal functions if they have the ACC_RETURN_REFERENCE flag set?

Why can't we always provide the retval ptr, even for functions that don't
return by reference? This would allow returning zvals without having to
copy them first (what RETVAL_ZVAL does).

Motivation for this is the following SO question:
http://stackoverflow.com/q/17844379/385378


Patch for this change can be found here:
https://github.com/php/php-src/pull/420

The patch also adds new macros to allow easy use of this feature called
RETVAL_ZVAL_FAST/RETURN_ZVAL_FAST (anyone got a better name?)

If no one objects I'll merge this sometime soon.
+1 Though looking through the ext uses, most functions returning an array build it directly in return_value and thus avoid the copy.  I also see that you've picked up all of the cases in ext/standard/array.c where these macros can be applied.


There's another one in string.c, in PHP_FUNCTION(pathinfo), that could be applied as well, though there's little performance gain in avoiding the copy of a 4 element string array.

BTW, looking at this pathinfo code, it doesn't do what the documentation says it does -- or at least this states that the optional argument if present should be _one_ of PATHINFO_DIRNAME, PATHINFO_BASENAME, PATHINFO_EXTENSION or PATHINFO_FILENAME. However, if a bitmask is supplied then this function returns the element corresponding to the lowest bit value rather than an error return, for example:

$ php -r 'echo pathinfo("/tmp/x.fred", PATHINFO_FILENAME|PATHINFO_EXTENSION),"\n";'
fred

This is a bizarre behaviour.   At a minimum the documentation should actually state what the function does. Or do we bother to raise a patch to fix this sort of thing, given that returning an empty string (or more consistently with other functions, NULL) in this case could create a BC break with existing buggy code?

Regards
Terry

Thread (10 messages)

« previous php.internals (#68692) next »