-
Notifications
You must be signed in to change notification settings - Fork 174
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
Removing auto-derived Ord impl for Locale/LangId #2142
Conversation
components/locid/src/langid.rs
Outdated
@@ -351,6 +351,18 @@ impl FromStr for LanguageIdentifier { | |||
} | |||
} | |||
|
|||
impl Ord for LanguageIdentifier { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: we can't agree on what the behavior of impl Ord for LanguageIdentifier
should be, so we don't want any impl of it. What things in ICU4X require it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the list of errors that I am running into if there is no Ord impl for LanguageIdentifier. This is probably not complete and most if not all of these errors are related to LiteMap.
provider/core/src/resource.rs
Outdated
@@ -383,6 +383,18 @@ impl fmt::Display for ResourceOptions { | |||
} | |||
} | |||
|
|||
impl Ord for ResourceOptions { | |||
fn cmp(&self, other: &Self) -> Ordering { | |||
self.strict_cmp(other.to_string().as_bytes()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: we should not call to_string()
, which is an expensive function, in cmp
, which should be efficient since it may be called many times.
Where do we need impl Ord for ResourceOptions
? Maybe we can change the call sites instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can switch to HashMap as you commented below. This is where I am getting errors for ResourceOptions if Ord impl is not provided.
https://github.com/unicode-org/icu4x/blob/main/provider/core/src/hello_world.rs#L110
https://github.com/unicode-org/icu4x/blob/main/provider/core/src/hello_world.rs#L127
Probably because HelloWorldProvider uses LiteMap: https://github.com/unicode-org/icu4x/blob/main/provider/core/src/hello_world.rs#L127
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HelloWorldProvider should just use HashMap, it doesn't matter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it can't, for reasons Rob mentioned
We could instead just use a manual LiteMap, a Vec
of tuples that we binary search
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved all of datagen to LiteMap a while ago in part because LiteMap has a bunch of utility functions that are useful for us that don't exist in HashMap.
This is a place where LocaleStr would be a good type to have.
I'm not quite sure what to recommend to unblock @snktd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think any of those utility functions you mentioned are still in use. See #2152, which removes LiteMap
deps and unblocks this.
@sffc I wonder if we should be using a regular hashmap instead here? |
It would be useful to see a list of the places and decide what to do there:
|
actually yeah a string might be fine |
Added the list of code-paths that throw errors if Ord impl is removed. |
All occurences of |
|
#2152 should have unblocked this |
Tried to fetch and update. There are still few more issues removing the Ord impl.
|
|
I don't think that's correct. We are following this algorithm, and assuming our current code is correct, changing to string comparison is not, because it changes the location of |
provider/core/src/data_provider.rs
Outdated
#[derive(Default, Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] | ||
#[derive(Default, Debug, PartialEq, Eq)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I see what's going on. You should not remove the Clone
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right! Thanks for catching! Updated.
Also, I am not entirely sure if this is the correct way to implement Ord. Let me know if I missed anything!
Fixes #1215