[Variant] Remove ceremony from iterator of variants into VariantArray #8625
Conversation
|
Re: #8611 (comment) I'll throw in the other |
friendlymatthew
force-pushed
the
friendlymatthew/impl-into-iter-variant-array
branch
from
1722f43 to
ece0ffb
Compare
| let mut b = VariantArrayBuilder::new(0); | ||
| b.extend(iter.into_iter().map(Some)); | ||
| b.build() |
There was a problem hiding this comment.
Would this work?
| let mut b = VariantArrayBuilder::new(0); | |
| b.extend(iter.into_iter().map(Some)); | |
| b.build() | |
| Self::from_iter(iter.into_iter().map(Some)) |
| Variant::ShortString(ShortString::try_new("norm").unwrap()), | ||
| ]; | ||
|
|
||
| let variant_array = VariantArray::from_iter(v.into_iter()); |
There was a problem hiding this comment.
Shouldn't need that into_iter call, the trait takes IntoIterator as input?
There was a problem hiding this comment.
Yeah I was just testing something and forgot to revert that. Luckily clippy caught it
| impl<'m, 'v> FromIterator<Option<Variant<'m, 'v>>> for VariantArray { | ||
| fn from_iter<T: IntoIterator<Item = Option<Variant<'m, 'v>>>>(iter: T) -> Self { | ||
| let mut b = VariantArrayBuilder::new(0); | ||
| b.extend(iter); | ||
| b.build() | ||
| } | ||
| } | ||
|
|
||
| impl<'m, 'v> FromIterator<Variant<'m, 'v>> for VariantArray { | ||
| fn from_iter<T: IntoIterator<Item = Variant<'m, 'v>>>(iter: T) -> Self { | ||
| let mut b = VariantArrayBuilder::new(0); | ||
| b.extend(iter); | ||
| b.build() | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I wonder if there's a way to get the iterator size without consuming the iterator...
But then again, I haven't ran a profile on the VariantArray code yet. I plan on looking at perf later on
There was a problem hiding this comment.
Most std lib code I've seen relies on the iterator's lower bound hint, with a fallback path if that turned out to be too low.
There's also the ExactSizeIterator trait, but given how rust treats potentially overlapping generic definitions as conflicts, I don't know how to provide implementations for both Iterator and ExactSizeIterator?
Yeah, I agree. I'll just remove the second impl From |
friendlymatthew
force-pushed
the
friendlymatthew/impl-into-iter-variant-array
branch
from
ece0ffb to
eda7457
Compare
friendlymatthew
force-pushed
the
friendlymatthew/impl-into-iter-variant-array
branch
from
eda7457 to
ea264c4
Compare
| } | ||
| } | ||
|
|
||
| impl<'m, 'v> FromIterator<Option<Variant<'m, 'v>>> for VariantArray { |
There was a problem hiding this comment.
If we do this:
| impl<'m, 'v> FromIterator<Option<Variant<'m, 'v>>> for VariantArray { | |
| impl<'m, 'v, V: Into<Variant<'m, 'v>> FromIterator<Option<V>> for VariantArray { |
then we can create a variant array from anything that converts cleanly to variant (e.g. i64 values or &str)?
There was a problem hiding this comment.
Yeah, I thought about this as well, but I figured we'd be creating 2 abstractions?
As in, I wonder if it's better to guide the user to do the Variant::from explicitly 🤔
There was a problem hiding this comment.
This is mostly going to be used by test code, and a homogenous set of e.g. i64 is a pretty common use case. But I don't have strong feelings either way.
2 participants
Which issue does this PR close?
VarianttoVariantArray#8606Rationale for this change
Mapping from an iterator of
Variants into aVariantArraycurrently requires a lot of ceremony. This PR abstracts over this.