Re: [PHP-DEV] Re: Latest ZE2 changes

[Zeev Suraski]

This does appear to be a problem, we'll need to think about it.

At 13:32 10/12/2002, Marcus Börger wrote:

At 10:02 10.12.2002, Zeev Suraski wrote:

At 15:44 08/12/2002, Marcus Börger wrote:

Looking into deep reveals that we must disallow overriding privates now.
That way the test private_007.phpt is illegal and all current tests i 
developed
would pass (visibility tests are suspended of cause).

How do you arrive in that conclusion?  The algorithm was designed to 
fully support it.  Running private_007 also appears to be working for me 
(there was a buglet in the parser with static+access level, but if you 
remove the statics and/or update to the latest CVS, you can see that it's 
working fine...)

Zeev

Yes 007 is fixed now but private_007b.phpt still fails because the wrong 
method is being called.
The problem i see is the following: You inherit private methods to be 
slightly faster in zend_compile.
In zend_execute you fetch the fbc from the object. Doing this you fetch 
the wrong method.

All above assumes we were linking privates statically as we do in case of 
static methods. Or do we
link all static mehtods statically and all dynamic methods dynamic? If so 
the test has to be corrected.

Maybe the latter is better since it would be hard to explain that privates 
are linked statically while
public and protected are linked dynamically. I wrote the test based on my 
first patches and did not
allow to change the visibility. In that case it makes no sense to 
dynamically link privates. Now
increasing visibility is allowed all should be linked dynamically, 
shouldn't they?

Here is the test 007b:

===ptivate_007b.php

class Bar {
public function pub() {
$this->priv();
}
private function priv() {
echo "Bar::priv()\n";
}
}
class Foo extends Bar {
public function priv()  {
echo "Foo::priv()\n";
}
}
$obj = new Foo();
$obj->pub();
$obj->priv();
echo "Done\n";
?>
===EOF
===ptivate_007b.log
 EXPECTED OUTPUT
Bar::priv()
Foo::priv()
Done
 ACTUAL OUTPUT
Foo::priv()
Foo::priv()
Done
 FAILED
===EOF


marcus


--
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] Re: Latest ZE2 changes

[Marcus Börger]

At 10:02 10.12.2002, Zeev Suraski wrote:

At 15:44 08/12/2002, Marcus Börger wrote:

Looking into deep reveals that we must disallow overriding privates now.
That way the test private_007.phpt is illegal and all current tests i 
developed
would pass (visibility tests are suspended of cause).

How do you arrive in that conclusion?  The algorithm was designed to fully 
support it.  Running private_007 also appears to be working for me (there 
was a buglet in the parser with static+access level, but if you remove the 
statics and/or update to the latest CVS, you can see that it's working fine...)

Zeev

Yes 007 is fixed now but private_007b.phpt still fails because the wrong 
method is being called.
The problem i see is the following: You inherit private methods to be 
slightly faster in zend_compile.
In zend_execute you fetch the fbc from the object. Doing this you fetch the 
wrong method.

All above assumes we were linking privates statically as we do in case of 
static methods. Or do we
link all static mehtods statically and all dynamic methods dynamic? If so 
the test has to be corrected.

Maybe the latter is better since it would be hard to explain that privates 
are linked statically while
public and protected are linked dynamically. I wrote the test based on my 
first patches and did not
allow to change the visibility. In that case it makes no sense to 
dynamically link privates. Now
increasing visibility is allowed all should be linked dynamically, 
shouldn't they?

Here is the test 007b:

===ptivate_007b.php

class Bar {
public function pub() {
$this->priv();
}
private function priv() {
echo "Bar::priv()\n";
}
}
class Foo extends Bar {
public function priv()  {
echo "Foo::priv()\n";
}
}
$obj = new Foo();
$obj->pub();
$obj->priv();
echo "Done\n";
?>
===EOF
===ptivate_007b.log
 EXPECTED OUTPUT
Bar::priv()
Foo::priv()
Done
 ACTUAL OUTPUT
Foo::priv()
Foo::priv()
Done
 FAILED
===EOF


marcus

Re: [PHP-DEV] Re: Latest ZE2 changes

[Zeev Suraski]

At 15:44 08/12/2002, Marcus Börger wrote:

Looking into deep reveals that we must disallow overriding privates now.
That way the test private_007.phpt is illegal and all current tests i 
developed
would pass (visibility tests are suspended of cause).

How do you arrive in that conclusion?  The algorithm was designed to fully 
support it.  Running private_007 also appears to be working for me (there 
was a buglet in the parser with static+access level, but if you remove the 
statics and/or update to the latest CVS, you can see that it's working fine...)

Zeev


--
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] Re: Latest ZE2 changes

[Marcus Börger]

At 00:41 08.12.2002, Marcus Börger wrote:

At 16:48 07.12.2002, Zeev Suraski wrote:

At 17:02 07/12/2002, Marcus Börger wrote:

Hi Zeev,

I have changed the test files and encountered some problems with the way 
you modified my patch:

1) private_002.phpt fails  with
004- Fatal error: Call to private method pass::show() from context 
'fail' in %s on line %d
004+ Fatal error: Call to public method fail::show() from context 'fail' 
in /usr/src/php4-HEAD/tests/classes/private_002.php on line 18

I see the problem.  It should be fixed.


Fixed now, thanks.




The new error message is wrong. We are trying to access pass::show and 
not fail::show().

2) private_007.phpt and private_007b.phpt both fail with the message
Fatal error: Cannot redeclare private bar::priv() as public foo::priv()

That means the children know about their parents privates what is in 
contrast to
our discussion. The reason they do is because you skipped the part of 
the patch
where in zend_do_inheritance() a modified version of 
zend_hash_merge_ex() was
used to prevent this. But you also do not search for the private method 
in the calling
scope. In other words you mainly used the first version of my patch. So 
i tried to
strip down zend_hash_merge_with_argument_if() to the portion we need and 
tried
to skip copying zje privates. But since privates are no longer looked up 
in the
calling scope this fails.

I implement PPP in a different way altogether (it's very loosely based on 
your patch, the actual implementation is very different), in order to 
avoid having two hash lookups for every method call.  I left a comment in 
the code that says we may want to improve the error messages, but the 
current implementation is quite intentional - it's much more efficient.

However this is about what i did in my first patches and gave up after 
discussion with Andi.
Since he was right that the performance loss by the required checks is 
outweighted by the
fact that otherwise classes know about private details of their ancestors. 
And i remeber having
an agreement on this. And yes you did the execute part in another way but 
until now i see
many problems in your way.


3) You left in some snippets of the 'final' patch. Does this mean we are 
going to have
final? A new patch is also available.

No, we're not going to add final, at least not for now.  I may have 
forgotten to remove the scanner part of the patch but it's meaningless :)

Zeev


After the patches today some more things need to be revisited:

4)  You allow changing the visibility. In most languages it is only allowed to
decrease the visibility or it is even disallowed. Increasing the 
visibility is a
very unfamiliar feature.

5) When you look at test private_007b.phpt you will see that the wrong
method is called. This is a consequence of 2. It seems you optimized to
much.


Looking into deep reveals that we must disallow overriding privates now.
That way the test private_007.phpt is illegal and all current tests i developed
would pass (visibility tests are suspended of cause).

If you agree and the current implementation is the way to go i can comit
the private and protected tests (not including private_007 of cause which
is to be discussed).



6) zend_execute.c, line 2350 should be
if (!(EX(fbc)->op_array.fn_flags & (ZEND_ACC_PROTECTED | 
ZEND_ACC_PRIVATE))) {
instead of
if (EX(fbc)->op_array.fn_flags & ZEND_ACC_PUBLIC) {
as we allow (fn_flags & ZEND_FN_PPP_MASK) == 0 being matched to public as 
well.

marcus


--
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php


--
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

Re: [PHP-DEV] Re: Latest ZE2 changes

[Marcus Börger]

At 16:48 07.12.2002, Zeev Suraski wrote:

At 17:02 07/12/2002, Marcus Börger wrote:

Hi Zeev,

I have changed the test files and encountered some problems with the way 
you modified my patch:

1) private_002.phpt fails  with
004- Fatal error: Call to private method pass::show() from context 'fail' 
in %s on line %d
004+ Fatal error: Call to public method fail::show() from context 'fail' 
in /usr/src/php4-HEAD/tests/classes/private_002.php on line 18

I see the problem.  It should be fixed.


Fixed now, thanks.




The new error message is wrong. We are trying to access pass::show and 
not fail::show().

2) private_007.phpt and private_007b.phpt both fail with the message
Fatal error: Cannot redeclare private bar::priv() as public foo::priv()

That means the children know about their parents privates what is in 
contrast to
our discussion. The reason they do is because you skipped the part of the 
patch
where in zend_do_inheritance() a modified version of zend_hash_merge_ex() was
used to prevent this. But you also do not search for the private method 
in the calling
scope. In other words you mainly used the first version of my patch. So i 
tried to
strip down zend_hash_merge_with_argument_if() to the portion we need and 
tried
to skip copying zje privates. But since privates are no longer looked up 
in the
calling scope this fails.

I implement PPP in a different way altogether (it's very loosely based on 
your patch, the actual implementation is very different), in order to 
avoid having two hash lookups for every method call.  I left a comment in 
the code that says we may want to improve the error messages, but the 
current implementation is quite intentional - it's much more efficient.

However this is about what i did in my first patches and gave up after 
discussion with Andi.
Since he was right that the performance loss by the required checks is 
outweighted by the
fact that otherwise classes know about private details of their ancestors. 
And i remeber having
an agreement on this. And yes you did the execute part in another way but 
until now i see
many problems in your way.


3) You left in some snippets of the 'final' patch. Does this mean we are 
going to have
final? A new patch is also available.

No, we're not going to add final, at least not for now.  I may have 
forgotten to remove the scanner part of the patch but it's meaningless :)

Zeev


After the patches today some more things need to be revisited:

4)  You allow changing the visibility. In most languages it is only allowed to
decrease the visibility or it is even disallowed. Increasing the visibility 
is a
very unfamiliar feature.

5) When you look at test private_007b.phpt you will see that the wrong
method is called. This is a consequence of 2. It seems you optimized to
much.

6) zend_execute.c, line 2350 should be
if (!(EX(fbc)->op_array.fn_flags & (ZEND_ACC_PROTECTED | 
ZEND_ACC_PRIVATE))) {
instead of
if (EX(fbc)->op_array.fn_flags & ZEND_ACC_PUBLIC) {
as we allow (fn_flags & ZEND_FN_PPP_MASK) == 0 being matched to public as well.

marcus


--
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php

[PHP-DEV] Re: Latest ZE2 changes

[Zeev Suraski]

At 17:02 07/12/2002, Marcus Börger wrote:

Hi Zeev,

I have changed the test files and encountered some problems with the way 
you modified my patch:

1) private_002.phpt fails  with
004- Fatal error: Call to private method pass::show() from context 'fail' 
in %s on line %d
004+ Fatal error: Call to public method fail::show() from context 'fail' 
in /usr/src/php4-HEAD/tests/classes/private_002.php on line 18

I see the problem.  It should be fixed.


The new error message is wrong. We are trying to access pass::show and not 
fail::show().

2) private_007.phpt and private_007b.phpt both fail with the message
Fatal error: Cannot redeclare private bar::priv() as public foo::priv()

That means the children know about their parents privates what is in 
contrast to
our discussion. The reason they do is because you skipped the part of the 
patch
where in zend_do_inheritance() a modified version of zend_hash_merge_ex() was
used to prevent this. But you also do not search for the private method in 
the calling
scope. In other words you mainly used the first version of my patch. So i 
tried to
strip down zend_hash_merge_with_argument_if() to the portion we need and tried
to skip copying zje privates. But since privates are no longer looked up 
in the
calling scope this fails.

I implement PPP in a different way altogether (it's very loosely based on 
your patch, the actual implementation is very different), in order to avoid 
having two hash lookups for every method call.  I left a comment in the 
code that says we may want to improve the error messages, but the current 
implementation is quite intentional - it's much more efficient.

3) You left in some snippets of the 'final' patch. Does this mean we are 
going to have
final? A new patch is also available.

No, we're not going to add final, at least not for now.  I may have 
forgotten to remove the scanner part of the patch but it's meaningless :)

Zeev


--
PHP Development Mailing List 
To unsubscribe, visit: http://www.php.net/unsub.php