Baked data for icu_collator
#3572
Conversation
robertbastian
requested review from
sffc and
Manishearth
and removed request for
hsivonen and
echeran
| DataPayload::from_static_ref( | ||
| icu_normalizer::provider::Baked::SINGLETON_NORMALIZER_NFD_V1, | ||
| ), | ||
| DataPayload::from_static_ref( | ||
| icu_normalizer::provider::Baked::SINGLETON_NORMALIZER_NFDEX_V1, | ||
| ), |
There was a problem hiding this comment.
Nit: For nested constructors I think we said that we wanted to call the globaldata constructor from the other crate rather than linking the data from that crate into this crate.
There was a problem hiding this comment.
I don't know if there's a public API we can go, at the moment this crate directly uses the DataPayloads.
I think what we decided was that if a crate calls a constructor from another crate, the data constructor should call other data constructors directly. This is not quite what's happening here.
There was a problem hiding this comment.
Still a bit uneasy; should we refactor the code to hold a Normalizer instead of the payloads, or perhaps create a Normalizer than then use a doc(hidden) API to extract the payloads from it? Let's discuss
There was a problem hiding this comment.
Discussion:
- If they are in different codegen units, it's possible that the data may be anchored in two different places (duplicated)
- But LTO or codegen-units=1 should fix it in most cases
- Most of the call sites are const constructors which have the same problem, but it will show up in client code rather than ICU4X code
Conclusion:
- Land this as-is and fix it later if we find it is an actual problem
robertbastian
force-pushed
the
collator
branch
3 times, most recently
from
135db95 to
1e434a7
Compare
|
Discuss with: Optional: |
#2945