Skip to content
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 unicode_ext_value! macro, enabled by new helper ShortVec #1767

Merged
merged 2 commits into from
Apr 6, 2022

Conversation

sffc
Copy link
Member

@sffc sffc commented Apr 2, 2022

This PR introduces ShortVec, an internal class to icu_locid, which looks like this:

/// Internal: A vector that supports no-allocation, constant values if length 0 or 1.
pub(crate) enum ShortVec<T> {
    Empty,
    Single(T),
    Multi(Vec<T>),
}

With this type, I can swap out the internals of Value to use ShortVec, and then, with a little bit of gymnastics due to rust-lang/rust#73255, I can get unicode_ext_value! to work:

const CALENDAR_KEY: Key = unicode_ext_key!("ca");
const CALENDAR_VALUE: Value = unicode_ext_value!("buddhist");

If you like this direction, we could:

  1. Keep it to just Value in Unicode and Transform
  2. Change all Vec to ShortVec

If we did (2), then, with some gymnastics, we could actually make the following compile, which seems cool:

// Potential future if ShortVec is used everywhere
const LOCALE: Locale = locale!("en-Latn-US-macos-u-ca-buddhist-t-hi-h0-hybrid");

However, this is a little further off, because it would require replacing LiteMap again, or changing LiteMap itself to use ShortVec. I lean against this generalization for the time being since it is generally expected that there are multiple Unicode extension keywords, whereas with the values, they are often only one subtag long.

We actually could already do locale!("en-t-hi") without changing anything in the data model.

@sffc sffc requested review from zbraniecki and nciric as code owners April 2, 2022 05:31
@sffc sffc requested review from robertbastian and removed request for nciric April 2, 2022 05:31
@sffc
Copy link
Member Author

sffc commented Apr 2, 2022

Note: Code size changes from 32072 to 32088. Seems like a worthwhile tradeoff.

@zbraniecki
Copy link
Member

How generic you want this new type to be? If it is intended for subtags only, maybe SubtagList for a name?

Copy link
Member

@zbraniecki zbraniecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@sffc sffc merged commit 1193250 into unicode-org:main Apr 6, 2022
@sffc sffc deleted the locale_short_vec branch April 6, 2022 00:49
@sffc
Copy link
Member Author

sffc commented Apr 6, 2022

How generic you want this new type to be? If it is intended for subtags only, maybe SubtagList for a name?

I don't want to take on the responsibility of exporting this type to be super generic, but I want it to be a bit more general than just SubtagList. I'm not far from making it be the LiteMap backend, for example.

@sffc
Copy link
Member Author

sffc commented Apr 6, 2022

Oops, I hit the merge button before @robertbastian reviewed 😞 Let me know if you have any feedback and I'll open a new PR for you.

@robertbastian
Copy link
Member

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants