Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:18202
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Message-ID: <15.85.33075.9C534034@pb1.pair.com>
To: internals@lists.php.net
Date: Thu, 18 Aug 2005 09:14:01 +0200
Organization: IWORKS
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.2) Gecko/20040803 [ http://iworks.at ]
MIME-Version: 1.0
References: <61.69.24081.E876BF24@pb1.pair.com>	<BF.79.33075.DBAACF24@pb1.pair.com>	<1997493019.20050816231952@marcus-boerger.de>	<65.2E.33075.C74E2034@pb1.pair.com> <101570804.20050817215854@marcus-boerger.de>
In-Reply-To: <101570804.20050817215854@marcus-boerger.de>
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] Re: [PATCH] internal class' static properties and constants
From: mike@php.net (Michael Wallner)

Hi Marcus Boerger, you wrote:

> Hello Michael,
> 
> Wednesday, August 17, 2005, 9:14:50 AM, you wrote:
>>>+       } else if (*property == value) {
>>>+               retval = SUCCESS;
>>>Here you compare the address of a zval, was that intended?
>>>In case you meant it, then i think it cannot be correct because that should
>>>never happen, or is that the case you actually want to prevent?
>>>In case you meant to prohibit from setting the same value again you need
>>>actual zval comparison which won't work easily for strings.
> 
>>This is from zend_reflection_api ReflectionProperty::setValue().
>>I assume it is meant to prevent useless operations,
>>because when else should the zvals point to the same address?
> 
> Yep in reflection api it is used for the exact same reason. I was only
> wondering whether that can happen here. Since your code is called outside
> request time it can only happen if some code tries to update the same prop
> with the same value twice.

It theoretically avoids 
	zpp = zend_std_get_static_property(...);
	zend_update_static_property(..., *zpp);
Also zend_std_write_property uses the same bit, so
I think better be on the safe side...


>>>+       } else {
>>>+               if (PZVAL_IS_REF(*property)) {
>>>+                       zval_dtor(*property);
>>>+                       (*property)->type = value->type;
>>>+                       (*property)->value = value->value;
>>>+
>>>+                       if (value->refcount) {
>>>+                               zval_copy_ctor(*property);
>>>+                       }
>>>+               } else {
>>>+                       **property = *value;
>>>+                       zval_copy_ctor(*property);
>>>+               }
>>>+               retval = SUCCESS;
>>>+       }
>>>+
>>>+       if (!value->refcount) {
>>>+               zval_dtor(value);
>>>+               FREE_ZVAL(value);
>>>+       }
>>>This only works because your zvals are initialized with refcount=0 which
>>>cannot be right. In the end it should always read here
>>>zval_ptr_dtor(&value); And tmp_zval() should initialize with refcount=1.
> 
> 
>>I actually thought that there should be an "else { zval_ptr_dtor(&value) }"?
>>Every place I looked at where "temporary zvals" are used, initialize them
>>with refcount=0.
> 
> 
> Temp zvals are used in scripts and can never be stored at least that was the
> goal. In your case always and unfortunatley also in the engine usage for
> edge cases they are going to be stored. So probably it is easier here to go
> with normal props initilized with refcount=1. You have to try and see.

zend_update_property() and zend_std_write_property() use the same form
of temporary zvals, though the destruction part of the passed value zval
should probably be adopted to that of zend_update_property().



>>They're not supposed to change their values after
>>declaration, are they?
> 
> 
> php -r '$d=42; define("a",$d); class t { const x=a;} var_dump(t::x);'
> 
> That's why.

So, what about disallowing IS_CONSTANT zvals for internal zvals as 
already done for arrays or other complex types and never actually
zval_update_constant() those zvals and skip internal classes in
zend_update_class_constants()?

Regards,
-- 
Michael - < mike(@)php.net >