Allow CodePointTrie to determine error_value at runtime from data #2301
Conversation
| #[zerofrom(clone)] // TrieValue is Copy, this allows us to avoid | ||
| // a T: ZeroFrom bound | ||
| pub(crate) error_value: T, |
There was a problem hiding this comment.
Suggestion: could we make a wrapper around T that implements ZeroFrom, something like
#[repr(transparent)]
pub struct CopyWrap<T: Copy>(pub T);
impl<T: Copy> ZeroFrom for CopyWrap<T> { ... }and export that wrapper from the ZeroFrom crate?
Given that we don't have specialization in Rust, wrapper structs like this seem like the best path forward in these types of situations.
There was a problem hiding this comment.
Eh, this is kinda what #[zerofrom(clone)] is for. We could introduce #[zerofrom(copy)] if we really want to be sure, but clone works fine here, it's a Copy type.
There was a problem hiding this comment.
#[zerofrom(clone)] has code smell. I like #[zerofrom(copy)], or my CopyWrap suggestion.
There was a problem hiding this comment.
Yeah but we can introduce that any time.
| @@ -12,19 +12,19 @@ const SAMPLE_STRING_LATIN1: &str = "Declaration loremips umdolo loremipsompi"; | |||
| const SAMPLE_STRING_MIXED: &str = "Dèclaråcion ЗАГАЛЬНА 世界人权宣言 𑄟𑄚𑄬𑄭𑄃𑄇𑄴𑄇𑄥𑄧𑄁𑄢𑄴"; | |||
|
|
|||
| fn get_trie_small() -> CodePointTrie<'static, u8> { | |||
| CodePointTrie::try_new( | |||
| CodePointTrie::try_new_with_error_at_end( | |||
There was a problem hiding this comment.
Issue: This is an iai bench and we do not want to allocate in it.
I suggest changing tries::gc_small to have the correct expanded form.
Basically, try to reduce the number of call sites of try_new_with_error_at_end. The only valid call sites I see are:
- toml.rs
- casemap try_from_icu
- maybe test_util.rs
There was a problem hiding this comment.
Done. Kept it in test_util because it's reading toml files
| pub struct CaseMappingData(u16); | ||
| #[cfg_attr(feature = "datagen", derive(serde::Serialize, databake::Bake))] | ||
| #[cfg_attr(feature = "datagen", databake(path = icu_casemapping::provider))] | ||
| pub struct CaseMappingData(pub u16); |
There was a problem hiding this comment.
Nit: keep the field private and implement CaseMappingData::const_default() if possible
Supersedes #2097, fixes #1879
I didn't get rid of
DATA_GET_ERROR_VALUEsince I'm calling itDEFAULT_ERROR_VALUE, and it can be used for empty CodePointTries.cc @pandusonu2 to see what changes I made