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

Fixed smallvec implementations to process elements inside. #82

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

xkikeg
Copy link
Contributor

@xkikeg xkikeg commented Jul 26, 2024

Now there's one drawback that in some case SmallVec won't be supported where A is still smallvec::Array but isn't [T; N]. However, it should be OK in reality, given smallvec itself is going to be SmallVec<T, N> instead of SmallVec<[T; N]> with removing this.

Context: servo/rust-smallvec#183

Closes #89

@fujiapple852
Copy link
Owner

fujiapple852 commented Jul 28, 2024

Hi @xkikeg and thanks for another PR!

So as I understand it:

  • smallvec 1.x is defined as SmallVec<A: Array> where Array is a trait with an Item associated type
  • smallvec 2.x is defined as SmallVec<T, const N: usize>

This PR improves the impl from:

impl<A> ToBoundedStatic for smallvec::SmallVec<A> 
where
    A: smallvec::Array + 'static,
    A::Item: Clone

To become:

impl<T, const N: usize> ToBoundedStatic for smallvec::SmallVec<[T; N]>
where
    [T; N]: smallvec::Array<Item = T>,
    [T::Static; N]: smallvec::Array<Item = T::Static>,
    T: ToBoundedStatic

This is clearly an improvement, as we can now convert any SmallVec<[T; N]> where T: ToBoundedStatic.

The limitation, as you note, is that it is restricting the usage to SmallVec<[T; N]> rather than the more general SmallVec<A> where A: Array. Not an issue for smallvec 2.x but could be a limitation for smallvec 1.x users.

I had a try and generalising it for the current smallvec 1.x version and came up with this (and similar for IntoBoundedStatic):

impl<A, T> ToBoundedStatic for smallvec::SmallVec<A>
where
    A: smallvec::Array<Item = T> + ToBoundedStatic,
    T: ToBoundedStatic,
    <A as ToBoundedStatic>::Static: smallvec::Array<Item = T::Static>,
{
    type Static = smallvec::SmallVec<A::Static>;

    fn to_static(&self) -> Self::Static {
        self.iter().map(ToBoundedStatic::to_static).collect()
    }
}

To make this work I also had to remove the Copy restriction from impl<T, const N: usize> ToBoundedStatic for [T; N] which is possible these days thanks to core::array::from_fn.

I changed:

impl<T, const N: usize> ToBoundedStatic for [T; N]
where
    T: ToBoundedStatic + Copy,
{
    type Static = [T::Static; N];

    fn to_static(&self) -> Self::Static {
        // Note that we required that `T` is `Copy` here whereas the `IntoBoundedStatic` impl does not.
        self.map(|item| item.to_static())
    }
}

To become:

impl<T, const N: usize> ToBoundedStatic for [T; N]
where
    T: ToBoundedStatic,
{
    type Static = [T::Static; N];

    fn to_static(&self) -> Self::Static {
        core::array::from_fn(|i| self[i].to_static())
    }
}

I pushed a WIP commit to this PR, what do you think?

Perhaps it would make sense to do this for smallvec 1.x and then move to your solution once smallvec 2.x is released? Alternately we could leave it as-is for now and then merge your solution once smallvec 2.x is released.

@xkikeg
Copy link
Contributor Author

xkikeg commented Jul 29, 2024

Thanks for suggestions! Your impl seems good so no objections for going with that approach (A: Array<Item=T>).

@fujiapple852 fujiapple852 merged commit ebcae04 into fujiapple852:master Aug 7, 2024
16 checks passed
@fujiapple852
Copy link
Owner

@xkikeg merged, thanks. Do you need this immediately? If not i'd prefer to hold off on a release until some other changes land.

@xkikeg xkikeg deleted the smallvec_internal branch August 7, 2024 12:21
@xkikeg
Copy link
Contributor Author

xkikeg commented Aug 7, 2024

Thanks! now I don't need this (foturunately or unfortunately Vec is slightly faster than SmallVec for my use case so I have no motivation to use SmallVec)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove 'static bound from T for ToBoundedStatic for smallvec::SmallVec<[T; N]>
2 participants