Skip to content

Fix mb_strimwidth RC info #9254

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Fix mb_strimwidth RC info #9254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c5e87c
Fix mb_strimwidth RC info
iluuu1994 Aug 5, 2022
ca487c1
Fix redundant ZSTR_VAL empty check
iluuu1994 Aug 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Zend/Optimizer/zend_func_infos.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,6 @@ static const func_info_t func_infos[] = {
F1("mb_strrichr", MAY_BE_STRING|MAY_BE_FALSE),
F1("mb_substr", MAY_BE_STRING),
F1("mb_strcut", MAY_BE_STRING),
F1("mb_strimwidth", MAY_BE_STRING),
F1("mb_convert_encoding", MAY_BE_ARRAY|MAY_BE_ARRAY_KEY_LONG|MAY_BE_ARRAY_KEY_STRING|MAY_BE_ARRAY_OF_ANY|MAY_BE_STRING|MAY_BE_FALSE),
F1("mb_convert_case", MAY_BE_STRING),
F1("mb_strtoupper", MAY_BE_STRING),
Expand Down
2 changes: 1 addition & 1 deletion ext/mbstring/mbstring.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -761,7 +761,7 @@ static PHP_INI_MH(OnUpdate_mbstring_http_input)
php_error_docref("ref.mbstring", E_DEPRECATED, "Use of mbstring.http_input is deprecated");
}

if (!new_value || !ZSTR_VAL(new_value)) {
if (!new_value) {
Copy link
Member

Choose a reason for hiding this comment

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

Did you change this deliberately? If so, the implementation of the function could be simplified; basically to an if-else, then swap the branches, then drop the else, since the then branch guards.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it produced a compiler warning (as the check is redundant) but I didn't want to change anything else.

Copy link
Member

Choose a reason for hiding this comment

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

Is that check really redundant? What about an empty zend_string (or more precisely, a zend_string where the first byte is NUL)?

Copy link
Member Author

Choose a reason for hiding this comment

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

@cmb69 Then it's still decayed to a pointer that points to a '\0'.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to demonstrate: https://godbolt.org/z/6x9z7d8a9

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like adding a new regression test is in order.

Copy link
Member Author

Choose a reason for hiding this comment

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

I assume this is a bug stemming from the time we changed from char* to zend_string*

Doesn't look like it. It was added here: 379d9a1 But maybe it was just always wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just tried checking what the manual says about mbstring.http_input: https://www.php.net/manual/en/mbstring.configuration.php#ini.mbstring.http-input

It doesn't document what is "supposed" to happen if mbstring.http_input is set to an empty string. I suggest we stick to whatever the historical behavior was.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, might be best for now. After all, these deprecated INI settings were supposed to be removed long ago.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll merge this without that commit for now.

const char *encoding = php_get_input_encoding();
MBSTRG(http_input_set) = 0;
_php_mb_ini_mbstring_http_input_set(encoding, strlen(encoding));
Expand Down
1 change: 0 additions & 1 deletion ext/mbstring/mbstring.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,6 @@ function mb_strcut(string $string, int $start, ?int $length = null, ?string $enc

function mb_strwidth(string $string, ?string $encoding = null): int {}

/** @refcount 1 */
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this hint is used for some kind of optimizations by the JIT? Probably for cases where the returned string doesn't "escape" and can be immediately freed at the end of a block?

I just looked through the entire list and the annotations look accurate for the other functions. There are some other cases where potentially we could just return the input string unmodified, which might help to reduce the number of dynamic allocations. If I ever do that I will remember to adjust the annotations here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do wonder why mb_scrub isn't annotated as @refcount 1. I believe that function should always return a newly allocated string with refcount 1. Or... is there something I'm missing?

function mb_strimwidth(string $string, int $start, int $width, string $trim_marker = "", ?string $encoding = null): string {}

/**
Expand Down
2 changes: 1 addition & 1 deletion ext/mbstring/mbstring_arginfo.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.