-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Major overhaul of mbstring (part 3) #6416
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
Conversation
c5a81c2
to
469f2bf
Compare
/* Two input byte sequences can translate to either a 'halfwidth' or a | ||
* 'fullwidth' version of a character; our implementation of SJIS-2004 | ||
* translates them to the fullwidth versions */ | ||
if (preg_match('/Fullwidth: U\+([0-9A-F]+)/', $line, $match)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this only affects two codepoints, maybe use the generic parsing function and overwrite them explicitly, as you did for other encodings? That would allow us to drop the comments from the input file, which is quite large (300 KB).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I like the fact that we use unmodified, "official" conversion tables as the basis for the tests. But if that is what you want, no sweat.
Just say "do it".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH I'm not sure on this one. Just a bit worried about bloating the distribution with large test-only data files. At least these should gzip well, so maybe I'm worried for nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻 for thinking about the impact on all PHP core developers, not just on mbstring
.
We could gzdeflate
the file before parsing it. It would mean that zlib
has to be enabled to run the mbstring
tests. It would also mean that if the files are ever modified (which is not likely) we wouldn't get nice diffs.
I believe that git
stores committed files in a compressed format, so the size of the repo may not increase that much either way. Of course, the size of a checkout will be a bit bigger.
Whichever way you prefer is fine.
Previously, the unit tests for these text encodings covered all mappings from legacy -> Unicode, and all _reversible_ mappings from Unicode -> legacy. However, we should also test the few Unicode -> legacy mappings which are not reversible.
Each flush function in a chain of mbstring conversion filters always calls the next flush function in the chain. So it is not necessary to explicitly flush the second filter in a chain. (Due to this bug, in many cases, flush functions were actually being called three times.)
This faulty binary search would never reject values at the very high end of the range being searched, even if they were not actually in the table. Among other things, this meant that some Unicode codepoints which do not correspond to any character in JIS X 0213 would be converted to bogus Shift-JIS-2004 values rather than being rejected.
… for Kanji correctly If the 2nd byte of a 2-byte character is invalid, then mb_substitute_character() should be respected. Instead, what mbstring was doing was 'swallowing' the first byte, then emitting the 2nd byte as if it was an ASCII character. Likewise, if the 2nd byte is missing, instead of just keeping quiet, report an illegal character as specified by mb_substitute_character().
… in SJIS-2004 Unicode has 'combining' characters which join with another following character. Japanese hiragana and katakana with the 'two dots' voice mark can be represented in this way, with one Unicode character for the 'base' kana and another one which adds the voice mark. In SJIS-2004, however, there are dedicated characters for voiced and unvoiced kana. So some special checks are done to identify sequences of Unicode characters which need to be 'collapsed' into a single SJIS-2004 character. If a kana is immediately followed by some other unrelated character, like a Cyrillic letter, then the cached kana should be output 'as is' and we proceed with encoding the unrelated character. When doing this, though, we need to re-initialize local variables, or else the unrelated character will be mangled in some cases.
…for Kanji correctly Also, don't accept 1st bytes above 0xED, since none of the possible 2-byte sequences starting with 0xEE and above are actually mapped to any character.
Since 1993, Unicode has had a specific codepoint for a fullwidth Yen sign. Likewise, MacJapanese has separate kuten codes for halfwidth and fullwidth Yen signs. But mbstring mapped _both_ Yen sign codepoints to the MacJapanese fullwidth Yen sign. It's probably more appropriate to map the 'ordinary' Yen sign to the MacJapanese halfwidth Yen sign. Besides, this means that the conversion between Unicode and MacJapanese is closer to being lossless and reversible.
…depoints When converting Unicode to MacJapanese, some special sequences of Unicode codepoints are collapsed into a single SJIS character. When the implementation sees a codepoint which *might* begin such a sequence, it is cached and examined again after the next codepoint arrives. If it turns out that it wasn't one of the 'special' sequences, then a 'fallback' conversion table is consulted to convert the cached codepoint. Then we re-enter the regular conversion code to convert the immediately following codepoint. BUT, local variables need to be reinitialized properly when doing this! Because the locals weren't reinitialized, the sad result was that some codepoints would get chopped up into bit salad and emitted as something totally bogus (which might not even be valid SJIS-mac text at all).
…a bad transcoding hint To give the background on this issue, here is an excerpt from JAPANESE.txt, from the Unicode Consortium: Apple has defined a block of 32 corporate characters as "transcoding hints." These are used in combination with standard Unicode characters to force them to be treated in a special way for mapping to other encodings; they have no other effect. Sixteen of these transcoding hints are "grouping hints" - they indicate that the next 2-4 Unicode characters should be treated as a single entity for transcoding. The other sixteen transcoding hints are "variant tags" - they are like combining characters, and can follow a standard Unicode (or a sequence consisting of a base character and other combining characters) to cause it to be treated in a special way for transcoding. These always terminate a combining-character sequence. The transcoding coding hints used in this mapping table are: 0xF860 group next 2 characters as a single entity for transcoding 0xF861 group next 3 characters as a single entity for transcoding 0xF862 group next 4 characters as a single entity for transcoding 0xF87A variant tag for "negative" (i.e. black & white reversed) 0xF87E variant tag for vertical form 0xF87F variant tag for other alternate form For example, the Apple addition character 0x85AB is Roman numeral thirteen. There is no single Unicode for this (although there are standard Unicodes for Roman numerals 1-12). Using the grouping hint 0xF862 in combination with standard Unicodes, we can map this as 0xF862+0x0058+0x0049+0x0049+0x0049 (i.e. X + I + I + I). Our SJIS-mac conversion code actually recognizes some special sequences which start with an Apple 'transcoding hint'. However, if a transcoding hint is misplaced and is not followed by one of the expected sequences, we can just emit one error marker for the bad transcoding hint and then process the following codepoint as normal.
For consistency with UTF-16 and UCS-4. Also, do some code cleanup.
- For consistency with UTF-16, UTF-32, and UCS-4, strip leading byte order marks. - Treat it as an error if string is truncated (i.e. has an odd number of bytes).
Just pushing an update. As always, thanks for the great review! |
469f2bf
to
a513aa1
Compare
We can squeeze out a lot of duplicated code in this way.
a513aa1
to
dec3eca
Compare
Landed. |
A continuation of #6052 and #6402.
These are mostly fixes for Shift-JIS-2004 and MacJapanese (SJIS-mac), along with a couple other things.
@nikic... FYA