im-util: Re-introduce ArrayVec for limit adapter buffering #44
Conversation
There was a problem hiding this comment.
I am not a fan of this, but I can live with it.
Can we please rename LimitBuf to ArrayBuffer and SortBuf to VecBuffer? It becomes generic and can be re-used by other stream adapters.
If we go that path, we can also renaùe VectorDiffContainerStreamLimitBuf to VectorDiffContainerStreamArrayBuffer and VectorDiffContainerStreamSortBuf to VectorDiffContainerStreamVecBuffer.
I've made a couple of suggestions during the review.
| /// [`VectorDiffContainer`]s' `LimitBuf`. | ||
| type VectorDiffContainerStreamLimitBuf<S> = | ||
| <<S as Stream>::Item as VectorDiffContainerOps<VectorDiffContainerStreamElement<S>>>::LimitBuf; | ||
|
|
||
| /// Type alias for extracting the buffer type from a stream of | ||
| /// [`VectorDiffContainer`]s' `SortBuf`. | ||
| type VectorDiffContainerStreamSortBuf<S> = | ||
| <<S as Stream>::Item as VectorDiffContainerOps<VectorDiffContainerStreamElement<S>>>::SortBuf; |
There was a problem hiding this comment.
| /// [`VectorDiffContainer`]s' `LimitBuf`. | |
| type VectorDiffContainerStreamLimitBuf<S> = | |
| <<S as Stream>::Item as VectorDiffContainerOps<VectorDiffContainerStreamElement<S>>>::LimitBuf; | |
| /// Type alias for extracting the buffer type from a stream of | |
| /// [`VectorDiffContainer`]s' `SortBuf`. | |
| type VectorDiffContainerStreamSortBuf<S> = | |
| <<S as Stream>::Item as VectorDiffContainerOps<VectorDiffContainerStreamElement<S>>>::SortBuf; | |
| /// [`VectorDiffContainer`]s' `ArrayBuffer`. | |
| type VectorDiffContainerStreamArrayBuffer<S> = | |
| <<S as Stream>::Item as VectorDiffContainerOps<VectorDiffContainerStreamElement<S>>>::ArrayBuffer; | |
| /// Type alias for extracting the buffer type from a stream of | |
| /// [`VectorDiffContainer`]s' `VecBuffer`. | |
| type VectorDiffContainerStreamVecBuffer<S> = | |
| <<S as Stream>::Item as VectorDiffContainerOps<VectorDiffContainerStreamElement<S>>>::VecBuffer; |
| type LimitBuf: Default; | ||
| type SortBuf: Default; |
There was a problem hiding this comment.
| type LimitBuf: Default; | |
| type SortBuf: Default; | |
| type ArrayBuffer: Default; | |
| type VecBuffer: Default; |
| make_diffs: impl FnMut(VectorDiff<T>) -> ArrayVec<VectorDiff<T>, 2>, | ||
| ) -> Option<Self>; | ||
|
|
||
| fn pop_from_limit_buf(buffer: &mut Self::LimitBuf) -> Option<Self>; |
There was a problem hiding this comment.
| fn pop_from_limit_buf(buffer: &mut Self::LimitBuf) -> Option<Self>; | |
| fn pop_from_array_buffer(buffer: &mut Self::ArrayBuffer) -> Option<Self>; |
|
|
||
| fn pop_from_limit_buf(buffer: &mut Self::LimitBuf) -> Option<Self>; | ||
|
|
||
| fn push_into_sort_buf( |
There was a problem hiding this comment.
| fn push_into_sort_buf( | |
| fn push_into_vec_buffer( |
| make_diffs: impl FnMut(VectorDiff<T>) -> SmallVec<[VectorDiff<T>; 2]>, | ||
| ) -> Option<Self>; | ||
|
|
||
| fn pop_from_buffer(buffer: &mut Self::Buffer) -> Option<Self>; | ||
| fn pop_from_sort_buf(buffer: &mut Self::SortBuf) -> Option<Self>; |
There was a problem hiding this comment.
| fn pop_from_sort_buf(buffer: &mut Self::SortBuf) -> Option<Self>; | |
| fn pop_from_vec_buffer(buffer: &mut Self::VecBuffer) -> Option<Self>; |
| @@ -12,21 +14,30 @@ pub trait VectorDiffContainerOps<T>: Sized { | |||
| f: impl FnMut(VectorDiff<T>) -> Option<VectorDiff<U>>, | |||
| ) -> Option<VectorDiffContainerFamilyMember<Self::Family, U>>; | |||
|
|
|||
| fn push_into_buffer( | |||
| fn push_into_limit_buf( | |||
There was a problem hiding this comment.
| fn push_into_limit_buf( | |
| fn push_into_array_buffer( |
What do you not like? The bit of code duplication in
I think something like that should only be done once another adapter is actually written that uses the same types. Note that |
Yes, the code “duplication” and the two dependencies. I understand why you prefer a static array: it statically asserts that we won't “overflow”/overgrow the buffer. I wish It's OK to not agree. In this case, the patch is fine and I'm OK to approve it too. |
|
Time for There is actually the Do you think it would make sense to feature-gate the individual adapters (or group of adapters like |
|
I don't believe it makes sense to feature-flag them for the moment. We don't have a lot of them. |
No description provided.