String to{Lower, Upper}Case handles Unicode special, non-locale dependent, casing rules #2098
Conversation
WojciechMazur
force-pushed
the
feature/string_toLUCase_special_handling
branch
from
aedbd20
to
4f0c1d5
Compare
| // Outside word boundary and not found cased | ||
| if (c.isWhitespace) false |
There was a problem hiding this comment.
Where does isWhitespace come from? I see no reference to whitespace in the Unicode spec for Final_Sigma. However I see the concept of "case-ignorable", which I don't see in the code.
We should stick 100% to the Unicode spec of Final_Sigma, and comment everything with the references that we're using to make the code directly comparable to the spec.
There was a problem hiding this comment.
You're right, it was a kind of simplification used to match with JVM, eg. matching only characters within word boundary. I've replaced it with the correct implementation based on Unicode specification. To make it possible I had to add two additional methods to Characters and associated arrays of range indices.
There was a problem hiding this comment.
You're right, it was a kind of simplification used to match with JVM, eg. matching only characters within word boundary.
Hum, wait. Do you mean that the JVM's toLowerCase() behaves differently than the Unicode spec? Do you have an example input string where it's not the same?
There was a problem hiding this comment.
Not really, by simplification I've meant simplification on our side, eg. test ```assertEquals("dσς aσς bσc", "DΣΣ AΣΣ BΣC".toLowerCase())`` was created as I've seen that only last sigma in each word should be changed and didn't know or interpretted incorectlly caseIgnorable rule from specification.
In each case I've tested on JVM (for Final_Sigma rule) incompatibilites with Unicode spec comes from implementation of different Unicode version in given JDK. For example there are 181 more "cased" symbols in Unicode 13.0.0 when comparing to 10.0.0 used in JDK 11. I belive it's fixed in newer versions.
WojciechMazur
force-pushed
the
feature/string_toLUCase_special_handling
branch
from
4f0c1d5
to
5827534
Compare
| @inline | ||
| def nextIndex(currentIdx: Int, currentChar: Char): Int = { | ||
| val charSize = Character.charCount(currentChar) | ||
| if (iterateForward) currentIdx + charSize | ||
| else currentIdx - charSize | ||
| } |
There was a problem hiding this comment.
This will not work for surrogate pairs, i.e., supplementary code points. You'll need something similar to skipCharsWithCombiningClassOtherThanNoneOrAboveForwards and its backwards counterpart. I suggest you follow the same pattern of having a pair of skipCaseIgnorableCharsForwards and skipCaseIgnorableCharsBackwards, written after the above for CombiningClass, and then use them in a similar way as is done in afterSoftDotted.
There was a problem hiding this comment.
When applying this change I've found out that we had a bug in skipCharsXY methods. Port from Scala.js have not took into account Strings created using String.substring method. I'm not sure had it works in Scala.js but in Scala Native it uses the same underlying byte array, but with different offset and count values to determinate start or end of string.
Following missing test would fail:
assertEquals("\u0307\u0301I",
"iíìĩı i\u0307\u0301i".substring(7).toUpperCase(Lithuanian)) // equivalent of "\u0307\u0301i"
assertEquals("I\u0301I",
"iíìĩı i\u0307\u0301i".substring(6).toUpperCase(Lithuanian)) // equivalent of "i\u0307\u0301i"
I've decided to put fix in this PR are they're strongly related and also main loop of skipCharsXY is used also for case ignorable characters
| // use offset + count instead of length to handle Strings created using subString method | ||
| val len = offset + count |
There was a problem hiding this comment.
length(), and codePointAt() should take care of offset. We shouldn't have to do this here. If we were manipulating value directly, we would have to do this; but when we're using length(), charAt, etc., we must not take offset into account, because those methods already do that. Perhaps codePointAt is wrong?
There was a problem hiding this comment.
I've reverted changes related to offset and count, I looks that problem with operations on substring was occouring due to some other bug which was already fixed in a meantime. Thanks you for for spotting this.
…e dependent, casing rules (scala-native#2098) This PR continues work originated in scala-native#2095 Scala.js port, becouse it have not contained handling for Unicode special handling for characters not being locale dpendent, as it is probably implemented natively in JS. Changes include handling of greek sigma letter, latin I with dot, and other characters specified in Unicode 13.0.0 SpecialCasing.txt. In order to make this PR fully compatible with Unicode specification there was introduced array of `cased` and `caseIgnorable` character indices used to determinate proper handling.
…e dependent, casing rules (scala-native#2098) This PR continues work originated in scala-native#2095 Scala.js port, becouse it have not contained handling for Unicode special handling for characters not being locale dpendent, as it is probably implemented natively in JS. Changes include handling of greek sigma letter, latin I with dot, and other characters specified in Unicode 13.0.0 SpecialCasing.txt. In order to make this PR fully compatible with Unicode specification there was introduced array of `cased` and `caseIgnorable` character indices used to determinate proper handling.
This PR continues work originated in #2095 Scala.js port, becouse it have not contained handling for Unicode special handling for characters not being locale dpendent, as it is probably implemented natively in JS.
Changes include handling of greek sigma letter, latin I with dot, and other characters specified in SpecialCasing.txt
Before converting from draft PR it needs to be rebased onto changes in #2095.