Repository navigation
[RFC] Add grapheme_str_pad() function - #25179
ChrisAbner wants to merge 10 commits into
Conversation
Like mb_str_pad(), but the target length is counted in grapheme clusters, so NFC and NFD forms of the same text, combining marks and emoji sequences are padded to the same visible length. The padding string is only cut on grapheme boundaries. Invalid UTF-8 makes it return false, like grapheme_strlen(). RFC: https://wiki.php.net/rfc/grapheme_str_pad
| goto overflow; | ||
| } | ||
|
|
||
| zend_string *result = zend_string_safe_alloc(1, ZSTR_LEN(input), left_bytes + right_bytes, false); |
There was a problem hiding this comment.
Should this be one of the fallible zend string functions? So that it respects the allowed memory size?
str_pad("a", PHP_INT_MAX, "b")givesFatal error: Allowed memory size of 134217728 bytes exhausted.grapheme_str_pad("a", PHP_INT_MAX, "b")givesHard memory limit of 268435456 bytes exhausted.
There was a problem hiding this comment.
Good catch, thanks. Fixed in f397ad9: it now uses zend_string_safe_falloc() with RETURN_OOM(), like str_pad() and mb_str_pad(), so your example hits the regular memory_limit error. I added grapheme_str_pad_oom.phpt for it.
Like str_pad() and mb_str_pad(), an oversized result now hits the regular memory_limit fatal error instead of the hard memory limit.
Fill the padding like mb_str_pad() does, with memset() for a single byte and by doubling the copied area otherwise, instead of one memcpy() per copy of the padding string. Measure the padding string in one pass and stop once enough grapheme clusters are counted, so a long padding string is not walked three times. Stop counting the input once it is longer than the target length, since it is then returned unchanged.
|
Hi @ChrisAbner, the RFC says invalid UTF-8 in $pad_string returns false, but it is only checked when padding is needed: var_dump(grapheme_str_pad("abc", 2, "\xff")); // string(3) "abc"it is fine behaviour wise I think, it matches the empty |
|
Thanks, agreed. Like mb_str_pad(), neither $pad_string nor $pad_type is checked when no padding is needed. I've updated the RFC to say so and added a test for both cases in 829161e. |
| return false; | ||
| } | ||
|
|
||
| ubrk_setUText(bi, ut, &ustatus); |
There was a problem hiding this comment.
I ll have a better overall look later but I think you need to check its status here.
There was a problem hiding this comment.
A successful call no longer leaves the error of a previous failed call, so intl_get_error_code() reflects the last call, like normalizer_normalize() and transliterator_transliterate().
Like grapheme_strlen(), convert the string with intl_convert_utf8_to_utf16(), which also rejects invalid UTF-8, and iterate the UTF-16 text, instead of validating with U8_NEXT() and iterating a UTF-8 UText. The byte length of a prefix of the padding string is taken from its UTF-16 offset. This is about a third faster on large non-ASCII strings and slightly faster on short ones.
|
Pushed three more commits besides 333837b. ed4eee9 adds a test for the two "String size overflow" checks. They had no test before. f164108 clears the intl error at the start of the function. normalizer_normalize() and transliterator_transliterate() already do this. No grapheme_* function does it yet. 48b7896 counts grapheme clusters through UTF-16 with intl_convert_utf8_to_utf16(), like grapheme_strlen(), instead of validating with U8_NEXT() and walking a UTF-8 UText. The status check stays on ubrk_setText(). In a release build it is about a third faster on large non-ASCII strings. About f164108: I can drop it if you prefer consistency with the other grapheme functions. If it looks fine, I can apply the same change to the other grapheme functions in a separate PR. |
|
I think deserve some test coverage. |
|
Added in e197470: one test per merge case listed in the RFC. The results are the same with every ICU version I tried, from 57 to 76. I also added 371d81c, which checks that invalid input is rejected even when no padding is needed. Happy to add more cases if you think of others. These are edge cases, but they should be covered. |
RFC: https://wiki.php.net/rfc/grapheme_str_pad
Adds
grapheme_str_pad()to ext/intl, the grapheme cluster counterpart ofstr_pad()andmb_str_pad().grapheme_strlen().mb_str_pad()(ValueErrorfor an empty padding string or an invalid$pad_type, no error when nothing needs padding). Invalid UTF-8 returnsfalseand sets the intl error, likegrapheme_substr()."\r\n"takes a fast path and gives the same result asstr_pad().Tests:
ext/intl/tests/grapheme_str_pad_basic.phpt(NFC/NFD, combining dakuten, Devanagari, emoji with modifier and ZWJ sequences, cuts of the padding string,"\r\n"),grapheme_str_pad_errors.phpt,grapheme_str_pad_oom.phptandgrapheme_str_pad_overflow.phpt.