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

Refactor mb_substitute_character() to use string|int fast ZPP check #5359

Merged
merged 1 commit into from May 11, 2020

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Apr 7, 2020

Targeting master and rework of #5348

I'm not sure what the best way was to handle int|string with ZPP so I came up with this horror... Any better idea or should I just use a zval and a switch block again?

@cmb69
Copy link
Contributor

cmb69 commented Apr 18, 2020

If I'm not mistaken, the sole parameter of mb_substitute_character() is actually string|int|null (the stub doc comment says it would be string|int). Since ZPP is not quite cheap performance wise, it may make sense to have 2 ZPPs (one checking for int and the other for ?string, or some other combination), but 4 ZPPs look over-engineered. However, I'm not sure whether this would be helpful, given the somewhat special argument requirements, and the inherent weak typing of internal functions. It might be best to just stick to the current type switch.

@Girgias
Copy link
Member Author

Girgias commented Apr 19, 2020

If I'm not mistaken, the sole parameter of mb_substitute_character() is actually string|int|null (the stub doc comment says it would be string|int). Since ZPP is not quite cheap performance wise, it may make sense to have 2 ZPPs (one checking for int and the other for ?string, or some other combination), but 4 ZPPs look over-engineered. However, I'm not sure whether this would be helpful, given the somewhat special argument requirements, and the inherent weak typing of internal functions. It might be best to just stick to the current type switch.

It's better described as string|!string (meaning string or anything which is not a string) as currently anything that is not a string (well even if it's a string but fails the comparison to one of the preassigned strings) will be silently converted to an integer, this includes null which is converted to 0.

I just thought of a trick to reduce it from 4 to 3 ZPP checks, but I think the way to solve this ludicrous over engineering is to have a Fast ZPP macro which can check if an argument is integer or string, in the same way as we have one for array|string

@Girgias
Copy link
Member Author

Girgias commented Apr 22, 2020

Well I decided to use a big switch instead and implement strict_types handling manually, still can't use actual type hints otherwise a ZPP mismatch is emitted.

@Girgias
Copy link
Member Author

Girgias commented May 2, 2020

This is now based on #5449

@Girgias Girgias force-pushed the mb-substitute-improve branch 2 times, most recently from 0daff00 to 514095b Compare May 3, 2020 02:44
@Girgias Girgias force-pushed the mb-substitute-improve branch 3 times, most recently from a4c9a2f to 94b1d8e Compare May 10, 2020 10:58
@Girgias Girgias changed the title Refactor mb_substitute_character() Refactor mb_substitute_character() to use string|int fast ZPP check May 10, 2020
@Girgias
Copy link
Member Author

Girgias commented May 11, 2020

@nikic can you review this?

}
break;
if (substitute_character != NULL) {
if (zend_string_equals_literal(substitute_character, "none")) {
Copy link
Member

Choose a reason for hiding this comment

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

These were case-insensitive previously, you can use _ci.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, nice catch!

}
/* Invalid string value */
zend_argument_value_error(1, "must be 'none', 'long' or 'entity'");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
zend_argument_value_error(1, "must be 'none', 'long' or 'entity'");
zend_argument_value_error(1, "must be 'none', 'long', 'entity' or a valid codepoint");

Maybe? As is, it kinda sounds like you can only pass in those strings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, adjusted the message accordingly.

Using the new Fast ZPP API for string|int|null

This also fixes Bug #79448 which was too disruptive to fix in PHP 7.x
@php-pulls php-pulls merged commit 7dd332f into php:master May 11, 2020
@Girgias Girgias deleted the mb-substitute-improve branch May 11, 2020 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants