Repository navigation
Strip control characters before canonical ordering - #378
Open
emincan2000 wants to merge 1 commit into
Open
emincan2000 wants to merge 1 commit into
emincan2000 wants to merge 1 commit into
Conversation
With STRIPCC the control characters were only removed in utf8proc_normalize_utf32, after utf8proc_decompose_custom had already sorted the combining marks and added the grapheme breaks. A control character between two runs of marks then left them next to each other in the wrong order, so the result wasn't NFC or NFD and mapping it again gave a different string. It also left a grapheme break before a mark that followed a removed control character. Drop those control characters in utf8proc_decompose_custom instead, so the result is the same as stripping them first.
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
With STRIPCC the control characters only get removed in utf8proc_normalize_utf32, after utf8proc_decompose_custom has already sorted the combining marks. So if a control char sits between two runs of marks, they end up next to each other in the wrong order. The result isn't NFC or NFD anymore and mapping it a second time gives a different string.
Repro: NFC with STRIPCC of "o̅\x01̨" gives "ǫ̅" but it should be "ǫ̅", which is what you get if you strip first and normalize after. With NFD the marks just stay out of order.
This drops those control chars in utf8proc_decompose_custom already, so the ordering and composition see the marks as adjacent. The newline handling (and HT to space) stays in normalize_utf32 since CRLF needs two code points. Two side effects, both the same as stripping first: a removed control char before a mark no longer leaves a grapheme break there with CHARBOUND (I added that case to the strip first then CHARBOUND test from #371), and CR + control char + LF now counts as one CRLF.
Tested: the new tests fail on master and pass with the fix. I also ran old vs new over ~100k random strings with a bunch of option combos. Without STRIPCC nothing changes, and with it the new output is identical to the old output on the same input with the control chars taken out first. The fuzzer ran clean for a few min under ASan and UBSan.