Implement Unicode Case Folding #229
Conversation
This commit adds CaseFoldedString as a partner to CIString. A CaseFoldedString is case folded according to the Unicode rules for Caseless Matching. In contrast to CIString, it does _not_ keep a reference to the input `String`. This commit changes CIString to be based on CaseFoldedString.
|
@armanbilge @rossabaker let me know what you think |
| * | ||
| * @param toString | ||
| * The original value the CI String was constructed with. | ||
| */ | ||
| final class CIString private (override val toString: String) | ||
| final class CIString private (override val toString: String, val asCaseFoldedString: CaseFoldedString) |
There was a problem hiding this comment.
This encoding eagerly folds Strings that may never have equals or hashCode called on them, and roughly double the memory per instance.
The current encoding caches the hash code on first use, but inside a Map would require refolding the String to test equality.
I can image either strategy winning in some contexts. But what ifasCaseFoldedString were lazy: we'd fold zero times when we can, once when we must. Lazy could either be a lazy val, or just a null-init pattern similar to the zero-init pattern already here on hashCode.
There was a problem hiding this comment.
@rossabaker yeah, that makes sense. I actually benchmarked def/lazy val/val. lazy val seemed like a reasonable tradeoff. I was hesitant because I think a lot of our use cases are hashCode/equality heavy, but I can make it a lazy val or use the null-init pattern.
There was a problem hiding this comment.
If you can stomach the var, null-init is the fastest unless multiple threads initialize it concurrently, which should be rare. This is howString#hashCode operates.
| * scala> res0.toUpperCase | ||
| * val res1: String = ἭΙ | ||
| * | ||
| * scala> res0.toUpperCase.toLowerCase == res0.toLowerCase |
There was a problem hiding this comment.
These methods are dangerous without an explicit Locale.
There was a problem hiding this comment.
Yeah, that's part of why my #232 implementation intentionally bypasses any JRE based string conversion and java.util.Locale stuff.
For example, from my reading on the Turkic rules around ı it sounds like they should apply to some Turkic languages, but not necessarily all Turkic languages. The Unicode standard points out these can include ISO-639-2 tur and aze, but wikipedia also includes kaz (Kazakh). I thought it might be best to just directly implement the rules, without using java.util.Locale to hint anything, and then let the any users interested in this behavior choose to use it under the assumption they would know better when they specifically they should apply.
There was a problem hiding this comment.
Oh, I was just pointing out that calling these methods without an explicit Locale is setting a bad example for others. I wish those overloads were deprecated.
There was a problem hiding this comment.
Ah, I see. I'll make sure anything like that is updated/removed in the final version.
| * @param turkicFoldingRules if `true`, use the case folding rules for | ||
| * applicable to some Turkic languages. | ||
| */ | ||
| def apply(value: String, turkicFoldingRules: Boolean): CaseFoldedString = { |
There was a problem hiding this comment.
How about just calling this one turkic and losing the boolean (and the overload). Are people going to need to toggle this with a boolean?
There was a problem hiding this comment.
I know I've created a bit of a mess by creating this and #232, but in that one I create separate types for Turkic and non-Turkic variants. I think if we support Turkic variants, we need to have separate types. I'm concerned that allowing two CaseFoldedString values to have the same String representation, but be created under different rules will be a source of rare, but terrible, bugs.
There was a problem hiding this comment.
I think they would need to be separate types. I guess we can support them, but it's locale-like behavior, and I'm not sure where it ends.
There was a problem hiding this comment.
Well...if we want to support Simple/Full Default/Canonical/Compatibility caseless matching, I feel like the Turkic variants aren't really the primary issue.
Unfortunately, it sounds like there are quite real use cases for most of these permutations 🙁
This commit adds CaseFoldedString as a partner to CIString. A CaseFoldedString is case folded according to the Unicode rules for Caseless Matching. In contrast to CIString, it does not keep a reference to the input
String.This commit changes CIString to be based on CaseFoldedString.
This is a draft because I still need to clean up tests, maybe add syntax for case folded literals.
Fixes #228