-
Notifications
You must be signed in to change notification settings - Fork 176
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
Less ZeroMap mutation in datagen #3098
Conversation
utils/zerovec/src/varzerovec/vec.rs
Outdated
@@ -405,6 +405,18 @@ impl<'a, T: VarULE + ?Sized, F: VarZeroVecFormat> VarZeroVec<'a, T, F> { | |||
} | |||
} | |||
|
|||
impl<A, T, F> From<Vec<A>> for VarZeroVec<'static, T, F> |
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.
not sure if we want an impliclty discarding conversion but i guess it's fine since From is known to be moving
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.
+1; prefer to not have the implicit moving conversion; where is it used? Seems better to change the call sites from myvec.into()
to (&myvec).into()
which is more explicit
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 mean, it's converting a Vec<T>
into a VarZeroVec<T>
, so no elements are discarded. Just because we don't move out of the Vec
doesn't mean we cannot offer this API for ergonomics as well. But reverted.
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)
Construct all
ZeroMap
s usingFromIterator
andBTreeMap
s. This reduces the issue in #2826 toZeroMap
'sFromIterator
.