Major overhaul of mbstring (part 23) #9042
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alexdowad
force-pushed
the
cleanup-mbstring-23
branch
from
e5ecd11 to
04ba80c
Compare
cmb69
reviewed
Jul 19, 2022
mb_convert_kana now uses the new text encoding conversion filters. Microbenchmarking shows speed gains of 50%-150% across various text encodings and input string lengths. The behavior is the same as the old mb_convert_kana except for one fix: if the 'zero codepoint' U+0000 appeared in the input, the old implementation would sometimes drop it, not passing it through to the output. This is now fixed.
alexdowad
force-pushed
the
cleanup-mbstring-23
branch
from
04ba80c to
6d3f5ca
Compare
|
Thanks to @cmb69 for the review. I may fuzz just a bit more, but in the meantime, review from others (if available) would be appreciated. |
|
I have done some more fuzzing; didn't find any bugs. |
|
If no-one else wants to review, I will merge tomorrow. |
nikic
reviewed
Jul 20, 2022
| static void mb_wchar_to_cp50222(uint32_t *in, size_t len, mb_convert_buf *buf, bool end); | ||
|
|
||
| /* See mbstring.c */ | ||
| uint32_t mb_convert_kana_codepoint(uint32_t c, uint32_t next, bool *consumed, uint32_t *second, int mode); |
There was a problem hiding this comment.
This is introducing a reverse-dependency from libmbfl to mbstring. Probably the function should continue to live in libmbfl?
There was a problem hiding this comment.
Oh, indeed. @alexdowad, can you please address this? If mb_convert_kana_codepoint() can be easily changed to be moved to libmbfl, an option might be to establish a callback.
There was a problem hiding this comment.
Will do (in my next PR, which is close to ready).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Now
mb_convert_kanahas been moved over to using the new text conversion filters.Fuzzing with a modified
sapi/fuzzer/php-fuzz-mbstringwas used to ensure there is no change from the old behavior (except for the fix for U+0000). A few known bugs in the old encoding conversion filters caused spurious fuzzer crashes; fixes for the old encoding conversion filters so as to suppress those as I continue moving forward.FYA @cmb69 @nikic @kamil-tekiela