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

Improved support for byte strings #72

Merged
merged 3 commits into from May 19, 2015
Merged

Conversation

borman
Copy link
Contributor

@borman borman commented May 8, 2015

Currently working on a serialization library for a format where all strings are byte strings. Thus, I find byte string support requiring some attention.

The proposed changes are:

  1. Allow to disassemble Bytes/ByteBuf into underlying representation
  2. Add a nice-looking Debug implementation for Bytes/ByteBuf
  3. Add methods to construct strings from byte strings via str::from_utf8
  4. Add default visit_byte_buf implementation as a fallback to visit_bytes
  5. Implement ValueDeserializer for Bytes/ByteBuf
  6. Recognize struct field names as byte strings.

pub fn as_vec(self) -> Vec<u8> {
self.bytes
}
}
Copy link
Member

Choose a reason for hiding this comment

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

why not also use Into here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would conflict with From<T> at line 94:

impl Into<Vec<u8>> for ByteBuf {...}
impl<T> From<T> for ByteBuf where T: Into<Vec<u8>> {...}
src/bytes.rs:100:1: 106:2 error: conflicting implementations for trait `core::convert::From` [E0119]
src/bytes.rs:100 impl<T> From<T> for ByteBuf where T: Into<Vec<u8>> {
src/bytes.rs:101     fn from(bytes: T) -> Self {
src/bytes.rs:102         ByteBuf {
src/bytes.rs:103             bytes: bytes.into(),
src/bytes.rs:104         }
src/bytes.rs:105     }
                 ...
src/bytes.rs:100:1: 106:2 note: conflicting implementation in crate `core`
src/bytes.rs:100 impl<T> From<T> for ByteBuf where T: Into<Vec<u8>> {
src/bytes.rs:101     fn from(bytes: T) -> Self {
src/bytes.rs:102         ByteBuf {
src/bytes.rs:103             bytes: bytes.into(),
src/bytes.rs:104         }
src/bytes.rs:105     }
                 ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would suggest removing

impl<T> From<T> for ByteBuf where T: Into<Vec<u8>>

in favor of simpler

impl From<Vec<u8>> for ByteBuf

Though that would be a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

does removing the From impl and adding the Into impl work?
Then the From impl is generated in the standard library if I read this correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then the From impl is generated in the standard library if I read this correctly.

Yes, the offending implementation is

impl<T> From<T> for T

http://doc.rust-lang.org/src/core/convert.rs.html#152

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

what I mean is, you don't need to implement Into, it's generated, since From exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, the message actually mentions From implementations, not Into.

Anyway, committed this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be generated if there was impl From<ByteBuf> for Vec<u8>, not impl From<Vec<u8>> for ByteBuf

Copy link
Member

Choose a reason for hiding this comment

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

oh right... I got very confused with all those From and Into... sorry about that

@borman
Copy link
Contributor Author

borman commented May 12, 2015

Updated; will squash commits after review.

erickt added a commit that referenced this pull request May 19, 2015
Improved support for byte strings
@erickt erickt merged commit adae2bd into serde-rs:master May 19, 2015
@erickt
Copy link
Member

erickt commented May 19, 2015

Thanks! I'm prepping for a release, so the WIPs are fine for me right now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants