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

Simplify Language representation #1695

Merged
merged 7 commits into from
Mar 16, 2022
Merged

Simplify Language representation #1695

merged 7 commits into from
Mar 16, 2022

Conversation

sffc
Copy link
Member

@sffc sffc commented Mar 15, 2022

This PR changes Language from wrapping Option<TinyAsciiStr> to just TinyAsciiStr, with the None variant encoded as a literal tinystr!("und").

There are two reasons for this change:

  1. It enables Language to be its own ULE.
  2. It should fix the sort order problem (Remove the Ord impl for LanguageIdentifier / Locale #1215).

Please let me know which benches you'd like me to run if you would like to see benchmark results.

Manishearth
Manishearth previously approved these changes Mar 15, 2022
components/locid/src/subtags/language.rs Outdated Show resolved Hide resolved
zbraniecki
zbraniecki previously approved these changes Mar 15, 2022
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.

Please, use is_empty, the rest looks good.

components/locid/src/subtags/language.rs Outdated Show resolved Hide resolved

const LANGUAGE_LENGTH: RangeInclusive<usize> = 2..=3;
const UND_VALUE: TinyAsciiStr<3> = tinystr!(3, "und");
// Safe because "und" is a valid language subtag
const UND: Language = Language(unsafe { TinyAsciiStr::from_bytes_unchecked(*b"und") });
Copy link
Member

Choose a reason for hiding this comment

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

This could be lang!("und") after #1631 . @zbraniecki please take a look at that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add a TODO(#348) here, and you can fix it in your PR (which it looks like you need to rebase anyway).

@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@sffc
Copy link
Member Author

sffc commented Mar 16, 2022

^ rebasing onto main.

Manishearth
Manishearth previously approved these changes Mar 16, 2022
@sffc sffc merged commit 0fd8cde into unicode-org:main Mar 16, 2022
@sffc sffc deleted the language-ule branch March 16, 2022 05:58
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.

None yet

4 participants