Add SchemaWrite and SchemaRead implementations for Cell and RefCell#257
Add SchemaWrite and SchemaRead implementations for Cell and RefCell#257adpthegreat wants to merge 14 commits intoanza-xyz:masterfrom
Conversation
- removed unnnecesary trait bounds on SchemaRead and SchemaWrite impls for Cell and RefCell, - removed redundant std feature flags
…l Reader and impl Writer
|
@cpubot reopened |
| const TYPE_META: TypeMeta = const { | ||
| match T::TYPE_META { | ||
| TypeMeta::Static { size, .. } => TypeMeta::Static { | ||
| size, | ||
| zero_copy: false, | ||
| }, | ||
| TypeMeta::Dynamic => TypeMeta::Dynamic, | ||
| } | ||
| }; |
There was a problem hiding this comment.
I believe all TYPE_META declarations could use T::TYPE_META.keep_zero_copy(false)
| const TYPE_META: TypeMeta = const { | |
| match T::TYPE_META { | |
| TypeMeta::Static { size, .. } => TypeMeta::Static { | |
| size, | |
| zero_copy: false, | |
| }, | |
| TypeMeta::Dynamic => TypeMeta::Dynamic, | |
| } | |
| }; | |
| const TYPE_META: TypeMeta = T::TYPE_META.keep_zero_copy(false); |
|
|
||
| unsafe impl<C: ConfigCore, T> SchemaWrite<C> for Cell<T> | ||
| where | ||
| T: SchemaWrite<C>, |
There was a problem hiding this comment.
I believe all these impls could include ?Sized on T
|
|
||
| #[test] | ||
| fn test_cell_basic() { | ||
| proptest!(proptest_cfg(), |(value: (u32, f32, i64, f64, bool, u8))| { |
There was a problem hiding this comment.
Given the the straightforward implementation of Cell, it's not clear we're getting much value out of testing all these type variations.
The implementation is basically cell.get() and Cell::new. What's being done here is likely better accomplished in the fuzzer
There was a problem hiding this comment.
I guess what we could test are the special cases like T != T::Dst / T::Src, T / T::Dst not being Sized
|
|
||
| #[test] | ||
| fn test_cell_char_bincode_equivalence() { | ||
| proptest!(proptest_cfg(), |(cell: Cell<char>)| { |
There was a problem hiding this comment.
Similar to the above, what are we getting out of a dedicated Cell<char> test that we aren't getting with Cell<u32>?
|
|
||
| #[test] | ||
| fn test_cell_arrays_bincode_equivalence() { | ||
| proptest!(proptest_cfg(), |(cell: Cell<[u32; 4]>)| { |
There was a problem hiding this comment.
Similar to the above, what are we getting out of a dedicated Cell<[u32; 4]> test that we aren't getting with Cell<u32>?
|
|
||
| #[test] | ||
| fn test_refcell_basic() { | ||
| proptest!(proptest_cfg(), |(value: (RefCell<u32>, RefCell<f32>, RefCell<i64>, RefCell<f64>, RefCell<bool>, RefCell<u8>))| { |
|
|
||
| type Nested = RefCell<Vec<u64>>; | ||
|
|
||
| proptest!(proptest_cfg(), |(vec in proptest::collection::vec(any::<u64>(), 0..=5))| { |
| } | ||
|
|
||
| #[test] | ||
| fn test_refcell_with_struct() { |
| fn size_of(src: &Self::Src) -> WriteResult<usize> { | ||
| let borrowed = src | ||
| .try_borrow() | ||
| .map_err(|_| crate::error::WriteError::Custom("RefCell already borrowed mutably"))?; |
There was a problem hiding this comment.
I think it makes sense to import WriteError in the file instead of using full qualified here
| #[inline] | ||
| fn read(reader: impl Reader<'de>, dst: &mut MaybeUninit<Self::Dst>) -> ReadResult<()> { | ||
| let val = T::get(reader)?; | ||
| dst.write(RefCell::new(val)); |
There was a problem hiding this comment.
I would prefer to use T::Dst::new(val) here - makes the code more uniform across implementations and forces proper types without context-based (return value)) inference
|
Alright will make these changes as requested |
3 participants
#110 reopened