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

Convert ext/standard/class_object to HNI #1625

Closed
wants to merge 3 commits into from

Conversation

simonwelsh
Copy link
Contributor

Also moves get_called_class() out of function and into this, as that's how the PHP docs groups it.

Part of #1480

@simonwelsh simonwelsh mentioned this pull request Jan 22, 2014
73 tasks
@scannell
Copy link
Contributor

I'm having issues with this because git keeps stripping the whitepsace when I git pull --rebase with this diff as HEAD which causes tests to fail. I'll pull this once I figure out how to get around that (or let me know if anyone has any ideas what could be wrong.)

@scannell
Copy link
Contributor

Also, please don't modify Zend tests -- if you do for some reason, you need to change the import script.

@simonwelsh
Copy link
Contributor Author

The Zend test modifications came from running the import script on a recent (last week sometime) pull of PHP-5.5. The only changes I made are also reflected in the import script (some norepo and importing another .inc file)

@scannell
Copy link
Contributor

I'm seeing some weird interaction with fb_intercept in our PHP code -- this is going to take a bit to unravel.

*/
<<__Native>>
function call_user_method_array(string $method_name,
object &$obj,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mixed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bah, another one of those? I'm going to make a script that checks the types match.

@scannell
Copy link
Contributor

Something in this pull is causing a consistent performance regression of about 1%. I'm not sure if it is HNI overhead or what, but we probably won't take this until this is resolved.

@scannell
Copy link
Contributor

Thanks to @jdelong I believe we've identified the source of the regression -- it's not your fault.

The FCallBuiltin logic needs to be fixed to support HNI functions first.
Also, get_defined_functions() needs to be fixed to show HNI functions as well. (The latter might be doable if you want to attempt it in a separate change.)

@simonwelsh
Copy link
Contributor Author

What's the status of this?

@ptarjan
Copy link
Contributor

ptarjan commented Mar 15, 2014

I tried to go through this today and rebasing it is an enormous undertaking. I got it to compile but I think I'm doing some wrong things semantically.

I also noticed it was failing some tests in test/zend/good/ext/standard/tests/class_object/ Can you make sure they all pass?

There also seems to be a behavior diff with this. Some of our tests are failing with mocked classes reporting their class as the mocked name instead of the original class.

@simonwelsh
Copy link
Contributor Author

Rebased. All quick/, zend/good/Zend and zend/good/ext/standard/tests/class_object/ are passing locally. Relies on #2221 for the checker script to pass.

@ptarjan
Copy link
Contributor

ptarjan commented Apr 1, 2014

This have lots of test failures in class_object still

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.

4 participants