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

Add implementation of Decode for Box<str> and Box<[T]> #565

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mversic
Copy link

@mversic mversic commented Jan 12, 2024

Closes #564

I can't think of a way to implement Decode for all unsized types

src/codec.rs Outdated Show resolved Hide resolved
src/codec.rs Outdated
@@ -1058,6 +1058,18 @@ impl Encode for str {
}
}

impl Decode for Box<str> {
fn decode<I: Input>(input: &mut I) -> Result<Self, Error> {
Ok(String::decode(input)?.into_boxed_str())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check the already existing Box Decode implementation. It should be in the same way as this implementation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think that decoded String will have some excess capacity or will it have capacity == len?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've taken a bit different route here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())
	}
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can I get a response here?

@mversic mversic force-pushed the boxed_slice branch 3 times, most recently from 984c99c to c1b1067 Compare January 13, 2024 10:01
@bkchr bkchr requested a review from koute February 5, 2024 06:26
src/codec.rs Outdated Show resolved Hide resolved
src/codec.rs Outdated Show resolved Hide resolved
src/codec.rs Show resolved Hide resolved
@mversic mversic force-pushed the boxed_slice branch 3 times, most recently from 23abd83 to e86febe Compare February 5, 2024 08:58
@mversic
Copy link
Author

mversic commented May 28, 2024

hi, this PR should be ready for merge. Can someone approve it now?

@nazar-pc
Copy link
Contributor

nazar-pc commented Oct 31, 2024

Would be great to get an impl for Arc<[T]> as well (see #633)

@mversic
Copy link
Author

mversic commented Nov 2, 2024

Would be great to get an impl for Arc<[T]> as well (see #633)

I don't want to complicate this PR further as it's not being reviewed as it is.
I'd be happy to implement Rc<[T]>/Arc<[T]> in a separate PR.

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.

Implement Decode for Box<T> where T: ?Sized
5 participants