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

When icu::locid::extensions::unicode::Value has a single-segment value, make it borrowable as &str #1764

Closed
hsivonen opened this issue Apr 1, 2022 · 7 comments
Labels
C-locale Component: Locale identifiers, BCP47 R-obsolete Resolution: This issue is no longer relevant

Comments

@hsivonen
Copy link
Member

hsivonen commented Apr 1, 2022

It's inefficient to write icu::locid::extensions::unicode::Value into a String in order to get a &str, especially when icu::locid::extensions::unicode::Value holds only one segment.

I suggest adding a method that returns Option<&str> with Some(...) if there is exactly single value segment and None otherwise. This would improve the API for single-segment use cases, such as collations.

@zbraniecki
Copy link
Member

hmm, treating single value segment differently from multi value segment seems to be leaking internal structure onto the API.

For extension types that hold only single part, we could do as_str() projecting it from TinyStr, but we can't extend it to multi-part values.

For multi-part, if your goal is comparisong, I think a compromise might be to add PartialEq<str> which splits the str by - and compares it to parts.

How does it sound?

@sffc
Copy link
Member

sffc commented Apr 1, 2022

How about changing the data model of unicode::Value to store a hyphen-delimited String.

@sffc
Copy link
Member

sffc commented Apr 2, 2022

Depending on the use case, I'd also be happy with a cmp_bytes on Value (or PartialEq<[u8]>, depending on #1709)

@hsivonen
Copy link
Member Author

hsivonen commented Apr 4, 2022

My goal isn't only comparison but also (in the case where special comparisons don't match) passing the extension value to the provider in the variant field of ResourceOptions.

@sffc
Copy link
Member

sffc commented Apr 4, 2022

My goal isn't only comparison but also (in the case where special comparisons don't match) passing the extension value to the provider in the variant field of ResourceOptions.

I have a draft PR open that changes ResourceOptions to support extensions::unicode::Value directly so that you don't need to convert to String.

@sapriyag sapriyag added the C-locale Component: Locale identifiers, BCP47 label Apr 14, 2022
@zbraniecki
Copy link
Member

In #1833 I had to figure out how to handle it. I was able to workaround this by just comparing to a const Value. I am a bit reluctant to forgo the value of Copy to store it as a String. I think 99% of cases it will be a single TinyStr and I'd like to optimize for that use case, while keeping the API working for both single item and multi item.

@sffc
Copy link
Member

sffc commented Apr 30, 2022

Yep; in #1750, icu_locid::extensions::unicode::Value is used more broadly in the data loading infrastructure, and I think we should just continue to use that type everywhere that locale preferences are being handled, rather than going back and forth to String.

@sffc sffc added the R-obsolete Resolution: This issue is no longer relevant label May 26, 2022
@sffc sffc closed this as not planned Won't fix, can't repro, duplicate, stale May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-locale Component: Locale identifiers, BCP47 R-obsolete Resolution: This issue is no longer relevant
Projects
None yet
Development

No branches or pull requests

4 participants