Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove special self/parent handling in get_class_name_map_ptr() #7330

Merged
merged 1 commit into from
Aug 10, 2021

Conversation

nikic
Copy link
Member

@nikic nikic commented Aug 2, 2021

zend_accel_get_class_name_map_ptr() for "self" and "parent" will
currently try to determine which class these refer to, and then
initialize the CE_CACHE on those strings.

However, this shouldn't be necessary: We already initialize
CE_CACHE on all class declaration names, so it should be covered
through that already.

zend_accel_get_class_name_map_ptr() for "self" and "parent" will
currently try to determine which class these refer to, and then
initialize the CE_CACHE on those strings.

However, this shouldn't be necessary: We already initialize
CE_CACHE on all class declaration names, so it should be covered
through that already.
@nikic
Copy link
Member Author

nikic commented Aug 2, 2021

@dstogov Did I miss something here, or is this dead code?

@dstogov
Copy link
Member

dstogov commented Aug 10, 2021

This is not a dead code.

I inserted asserts into the current code and caught a number of "failed" tests. e.g. in Zend/tests/bug43332_1.phpt self is not going to be cached.

<?php
namespace foobar;

class foo {
  public function bar(self $a) { }
}

Probably, this may be fixed by compile-time substitution of self to the real class name.

@nikic
Copy link
Member Author

nikic commented Aug 10, 2021

I'm not sure I get what you mean. The "self" string itself is also not cached before this patch. It adds a cache slot to "foobar\bar", which does not help the type resolution, and will be allocated for the class declaration already.

@dstogov
Copy link
Member

dstogov commented Aug 10, 2021

you are right.

@nikic nikic merged commit 5e997ec into php:master Aug 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants