-
Notifications
You must be signed in to change notification settings - Fork 175
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
Add icu_properties feature to icu_normalizer #5551
Conversation
c551389
to
a901761
Compare
|
Thanks. This needs testing with Pernosco to see if this breaks Pernosco's special knowledge about the ICU4X types involved here. I'll test and will report back. |
I created a new crate that depends on |
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.
This changeset with or without the feature enabled breaks Pernosco's rendering of the CharacterAndClass
type.
Without this PR:
self ::CharacterAndClass*@0x7285c15ff1b8={char=‘̧’ 0x327, ccc=(202 ‘Ê’)}
With this PR:
self ::CharacterAndClass*@0x7eacb49ff1b8=(3388998439)
I think this should not be merged as-is.
I'm not quite sure what precisely Pernosco wants to see. I suggest putting the accessors on CharacterAndClass
that have CanonicalCombiningClass
in the return value behind the feature and introducing differently-named u8
accessors. (I haven't yet tested if that would work.)
I pushed a change to restore Pernosco compat. (That took way more rounds of experimentation than I expected!) However, the above observation still applies: |
components/normalizer/src/lib.rs
Outdated
@@ -180,12 +196,12 @@ fn decomposition_starts_with_non_starter(trie_value: u32) -> bool { | |||
/// | |||
/// The trie value must not be one that signifies a special non-starter | |||
/// decomposition. (Debug-only) | |||
fn ccc_from_trie_value(trie_value: u32) -> CanonicalCombiningClass { | |||
fn ccc_from_trie_value(trie_value: u32) -> u8 { |
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.
nit: define type CanonicalCombiningClass = u8
or a newtype, and then qualify the properties one?
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.
This is a good idea, I did one better, I wrote a seamless shim type CanonicalCombiningClass(pub u8)
that gets imported in non-properties mode.
It's because I didn't switch to |
3032abf
to
2fb710a
Compare
647f507
to
f05bcf2
Compare
f05bcf2
to
6c92b79
Compare
Fixes #5121