-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Improve DynamicStruct::insert #11068
Improve DynamicStruct::insert #11068
Conversation
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.
Makes sense, and avoids unnecessary conversions.
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.
LGTM! just one tiny comment.
pub fn insert_boxed<'a>(&mut self, name: impl Into<Cow<'a, str>>, value: Box<dyn Reflect>) { | ||
let name: Cow<str> = name.into(); | ||
if let Some(index) = self.field_indices.get(&name) { | ||
self.fields[*index] = value; | ||
} else { | ||
self.fields.push(value); | ||
self.field_indices | ||
.insert(Cow::Owned(name.clone().into_owned()), self.fields.len() - 1); | ||
self.field_names.push(Cow::Owned(name.into_owned())); |
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.
is there a reason you dropped the usage of the entry api? to me it's more readable in this case.
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 can't use the entry API because it always requires an owned String
regardless of whether it needs it or not.
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 good point.
self.field_indices | ||
.insert(Cow::Owned(name.clone().into_owned()), self.fields.len() - 1); | ||
self.field_names.push(Cow::Owned(name.into_owned())); |
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.
self.field_indices | |
.insert(Cow::Owned(name.clone().into_owned()), self.fields.len() - 1); | |
self.field_names.push(Cow::Owned(name.into_owned())); | |
let name = name.into_owned(); | |
self.field_indices | |
.insert(Cow::Owned(name.clone()), self.fields.len() - 1); | |
self.field_names.push(Cow::Owned(name)); |
definitely not a cow expert, but this would feel better to me
# Objective I wanted to pass in a `String` to `DynamicStruct::insert_boxed` but it took in a &str. That's fine but I also saw that it immediately converted the `&str` to a `String`. Which is wasteful. ## Solution I made `DynamicStruct::insert_boxed` take in a `impl Into<Cow<str>>`. Same for `DynamicStruct::insert`. --- ## Changelog - `DynamicStruct::insert_boxed` and `DynamicStruct::insert` now support taking in anything that implements `impl Into<Cow<str>>`.
# Objective I wanted to pass in a `String` to `DynamicStruct::insert_boxed` but it took in a &str. That's fine but I also saw that it immediately converted the `&str` to a `String`. Which is wasteful. ## Solution I made `DynamicStruct::insert_boxed` take in a `impl Into<Cow<str>>`. Same for `DynamicStruct::insert`. --- ## Changelog - `DynamicStruct::insert_boxed` and `DynamicStruct::insert` now support taking in anything that implements `impl Into<Cow<str>>`.
Objective
I wanted to pass in a
String
toDynamicStruct::insert_boxed
but it took in a &str. That's fine but I also saw that it immediately converted the&str
to aString
. Which is wasteful.Solution
I made
DynamicStruct::insert_boxed
take in aimpl Into<Cow<str>>
. Same forDynamicStruct::insert
.Changelog
DynamicStruct::insert_boxed
andDynamicStruct::insert
now support taking in anything that implementsimpl Into<Cow<str>>
.