Add Buffer::slice_ref #2072
Add Buffer::slice_ref #2072
Conversation
vortex-buffer/src/buffer.rs
Outdated
| impl<T> PartialEq for Buffer<T> { | ||
| fn eq(&self, other: &Self) -> bool { | ||
| self.bytes == other.bytes | ||
| } | ||
| } | ||
|
|
||
| impl<T> Eq for Buffer<T> {} | ||
|
|
||
| impl<T> PartialOrd for Buffer<T> { | ||
| fn partial_cmp(&self, other: &Self) -> Option<Ordering> { | ||
| self.bytes.partial_cmp(&other.bytes) | ||
| } | ||
| } | ||
|
|
||
| impl<T> Hash for Buffer<T> { | ||
| fn hash<H: Hasher>(&self, state: &mut H) { | ||
| self.bytes.as_ref().hash(state) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I think previously we were wrong where we would compare alignment and values
| pub struct Buffer<T> { | ||
| pub(crate) bytes: Bytes, | ||
| pub(crate) length: usize, | ||
| pub(crate) alignment: Alignment, | ||
| pub(crate) _marker: std::marker::PhantomData<T>, | ||
| } | ||
|
|
||
| impl<T> PartialEq for Buffer<T> { | ||
| fn eq(&self, other: &Self) -> bool { | ||
| self.bytes == other.bytes |
There was a problem hiding this comment.
I don't really know what the Rust convention is here. Should the default compare everything (including alignment) and an extra function eq_bytes ignores it? Or the other way around?
vortex-buffer/src/buffer.rs
Outdated
| @@ -10,14 +12,32 @@ use crate::debug::TruncatedDebug; | |||
| use crate::{Alignment, BufferMut, ByteBuffer}; | |||
|
|
|||
| /// An immutable buffer of items of `T`. | |||
| #[derive(Clone, PartialEq, Eq, PartialOrd, Hash)] | |||
| #[derive(Clone, Eq)] | |||
| /// # Panics: | ||
| /// Requires that the given sub slice is in fact contained within the Bytes buffer; otherwise this function will panic. | ||
| pub fn slice_ref(&self, subset: &[T]) -> Self { | ||
| let subset_u8 = |
There was a problem hiding this comment.
You're forgetting to check the new alignment.
See slice - you cannot end up with a Buffer that doesn't respect the defined alignment so you must also panic if the subset_u8 isn't aligned per the current alignment.
There was a problem hiding this comment.
yes this is a bug, though we want to support slicing with alignment between align_of:: and self.alignment. In particular for varbinviews the buffer will have alignment of 16 but the slice will have alignment of at most 4.
And fix PartialEq, PartialOrd, Hash for Buffer
fix #2009