Deprecation of functions View as Markdown

Hello internals,

i digged a bit deeper regarding the assumption that zend_call_function()
is missing the abstract check and it shows that right now we delay error
generation. The following back trace shows that actually we have a special
opcode that is necessary to do so:

#0 zend_error (type=1, format=0x86adbf8 "Cannot call abstract method %v::%v()") at /usr/src/php-cvs/Zend/zend.c:1363
#1 0x084736de in ZEND_RAISE_ABSTRACT_ERROR_SPEC_HANDLER (execute_data=0xbf950114, tsrm_ls=0x878c248) at /usr/src/php-cvs/Zend/zend_vm_execute.h:461
#2 0x0847140c in execute (op_array=0x8946c10, tsrm_ls=0x878c248) at /usr/src/php-cvs/Zend/zend_vm_execute.h:92
#3 0x0843335a in zend_call_function (fci=0xbf950344, fci_cache=0x0, tsrm_ls=0x878c248) at /usr/src/php-cvs/Zend/zend_execute_API.c:981

That said i think moving the abstract test into zend_call_function() just
like it is inside the function call helper opcode is the way to go.

regards
marcus

Sunday, February 19, 2006, 11:57:04 AM, you wrote:

Hello internals,

i just made up a tiny patch that allows us to deprecate functions. The
background is that in the past we changed to issue E_STRICT or E_NOTICE
for stuff we are going to change in later versions. This is right now not
easily possible when we change to replace a functions name. The usual way
to add the new name is to rename the function and add an alias with the old
name but for the deprecation message we would need to have the function
implementation twice so that one can issue the message. The patch now
allows us to specify a function flag on the alias.

I checked an early version of the patch with Andi and he didn't like the
introduction of another if(). For the explicit calling handler i see no
problem in adding it since it is just an integer comparision which makes
out less than 0.1% of the zend_call_function() for the function-call
opcode i hid the check behind the abstract check. So there is no new
penalty. And actually i think the abstract check is neccessary for the
the other part also. So the patch found an error and doesn't come with any
additional penalty at all.

Patch is available for both 5.1 and HEAD here:
http://php.net/~helly/php/ext/ze2/ze2-deprecated-20060219-5_1.diff.txt
http://php.net/~helly/php/ext/ze2/ze2-deprecated-20060219-HEAD.diff.txt

Anyone against?

Marcus Boerger schrieb:

Anyone against?

No, I am +1 for a sane way to mark functions/methods as deprectated.

--
Sebastian Bergmann http://www.sebastian-bergmann.de/
GnuPG Key: 0xB85B5D69 / 27A7 2B14 09E4 98CD 6277 0E5B 6867 C514 B85B 5D69

Anyone against?

Wasn't against it two years ago when I last suggested it so I'm all for it
now :)

+1

I do like the way you've combined the deprecation check with the abstract
check, very sleek and cleanly addresses the performance concerns raised in
regards to this sort of thing.

The one thing I'd like to see tacked on is a define for PHP_DEP_FE(),
PHP_DEP_FALIAS(), etc...

-Sara

Marcus Boerger wrote:

i just made up a tiny patch that allows us to deprecate functions. The
background is that in the past we changed to issue E_STRICT or E_NOTICE
for stuff we are going to change in later versions. This is right now not

I noted that the patches use E_NOTICE while I was assuming that
compatibility warnings are the domain of E_STRICT.

Out of curiosity: Can someone quickly list what the consensus on the
meaning of the different error levels are?

Thanks,

• Chris

Just for the record I've been spending quite a bit of time, trying to
see how we can lower the amount of branches within our opcodes.
Although one if() doesn't make a big difference on its own, the large
number of branches we have do make a difference. So before you say
it's just an integer comparison or 0.1%, there is actually much more
to it. Also, the more branches we will have over time, the harder to optimize.

I wasn't against the deprecated feature but just stated that
implementation wise, adding another branch should be thought about
again. There are other ways of adding deprecated to functions, which
would only slow down the deprecated functions and not all functions.

That said, as the abstract patch truly solves a problem, and adding
the deprecated one on top of that isn't any slower, I don't mind the patch.

Marcus, as usual you send a patch for comment on the weekend, and
commit before you give fair amount of time for review. Monday is a
holiday here in the US, and generally speaking, giving 2-3 days is
the right thing to do. I've asked you numerous time and I suggest you
respect it in future (although I realize it was probably done on purpose).

Andi

At 02:57 AM 2/19/2006, Marcus Boerger wrote:

Hello internals,

i just made up a tiny patch that allows us to deprecate functions. The
background is that in the past we changed to issue E_STRICT or E_NOTICE
for stuff we are going to change in later versions. This is right now not
easily possible when we change to replace a functions name. The usual way
to add the new name is to rename the function and add an alias with the old
name but for the deprecation message we would need to have the function
implementation twice so that one can issue the message. The patch now
allows us to specify a function flag on the alias.

I checked an early version of the patch with Andi and he didn't like the
introduction of another if(). For the explicit calling handler i see no
problem in adding it since it is just an integer comparision which makes
out less than 0.1% of the zend_call_function() for the function-call
opcode i hid the check behind the abstract check. So there is no new
penalty. And actually i think the abstract check is neccessary for the
the other part also. So the patch found an error and doesn't come with any
additional penalty at all.

Patch is available for both 5.1 and HEAD here:
http://php.net/~helly/php/ext/ze2/ze2-deprecated-20060219-5_1.diff.txt
http://php.net/~helly/php/ext/ze2/ze2-deprecated-20060219-HEAD.diff.txt

Anyone against?

Best regards,
Marcus