Add implementation of Decode for Box<str> and Box<[T]> #565
Conversation
src/codec.rs
Outdated
|
|
||
| impl Decode for Box<str> { | ||
| fn decode<I: Input>(input: &mut I) -> Result<Self, Error> { | ||
| Ok(String::decode(input)?.into_boxed_str()) |
There was a problem hiding this comment.
Can you check the already existing Box Decode implementation. It should be in the same way as this implementation.
There was a problem hiding this comment.
Do you think that decoded String will have some excess capacity or will it have capacity == len?
There was a problem hiding this comment.
I've taken a bit different route here
There was a problem hiding this comment.
should I maybe use WrapperTypeDecode like this:
impl WrapperTypeDecode for Box<str> {
type Wrapped = String;
fn decode_wrapped<I: Input>(input: &mut I) -> Result<Self, Error> {
// Guaranteed to create a Vec with capacity == len
let vec = Vec::from(Box::<[u8]>::decode(input)?);
// Guaranteed not to reallocate the vec, only transmute to String
let str = String::from_utf8(vec).map_err(|_| "Invalid utf8 sequence")?;
// At this point we have String with capacity == len,
// therefore String::into_boxed_str will not reallocate
Ok(str.into_boxed_str())
}
}
mversic
force-pushed
the
boxed_slice
branch
3 times, most recently
from
984c99c to
c1b1067
Compare
mversic
force-pushed
the
boxed_slice
branch
3 times, most recently
from
23abd83 to
e86febe
Compare
|
hi, this PR should be ready for merge. Can someone approve it now? |
|
Would be great to get an impl for |
mversic
force-pushed
the
boxed_slice
branch
2 times, most recently
from
74e011e to
8d64040
Compare
I don't want to complicate this PR further as it's not being reviewed as it is. |
Signed-off-by: Marin Veršić <marin.versic101@gmail.com>
|
@koute could you give this another look please. |
| // Guaranteed not to reallocate the vec, only transmute to String | ||
| let str = String::from_utf8(vec).map_err(|_| "Invalid utf8 sequence")?; | ||
|
|
||
| assert_eq!(str.capacity(), str.len()); |
There was a problem hiding this comment.
| assert_eq!(str.capacity(), str.len()); | |
| debug_assert_eq!(str.capacity(), str.len()); |
There was a problem hiding this comment.
Maybe we could also have some sort of check that the underlying pointer did not change and there wasn't any reallocation.
| } | ||
|
|
||
| // Decoding succeeded, so let's get rid of `MaybeUninit`. | ||
| // TODO: Use `Box::assume_init` once that's stable. |
| } | ||
|
|
||
| // Decoding succeeded, so let's get rid of `MaybeUninit`. | ||
| // TODO: Use `Box::assume_init` once that's stable. |
There was a problem hiding this comment.
As @bkchr mentioned, use Box::assume_init since it's stable now (MSRV is 1.82.0, which is old enough to use)
There was a problem hiding this comment.
using this would require you to bump MSRV
current MSRV (Minimum Supported Rust Version) is
1.79.0but this item is stable since1.82.0
you may want to conditionally increase the MSRV considered by Clippy using theclippy::msrvattribute
for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#incompatible_msrv
#[warn(clippy::incompatible_msrv)]on by default [incompatible_msrv]
There was a problem hiding this comment.
I think it's fine to bump the MSRV here; 1.82 should be old enough at this point.
| // | ||
| // The explicit types here are written out for clarity. | ||
| // | ||
| // TODO: Use `Box::new_uninit_slice` once that's stable. |
There was a problem hiding this comment.
This is stable already, so use it.
| for elem in &mut *boxed_slice { | ||
| T::decode_into(input, elem)?; | ||
| } |
There was a problem hiding this comment.
This is gonna be mega slow for primitive types. We should use the same trick that we use in e.g. Decode for [T; N] or decode_vec_with_len where we read the data in bulk if T is a primitive type, otherwise this will be a huge performance trap.
5 participants
Closes #564
I can't think of a way to implement
Decodefor all unsized types