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

Serializer and Deserializer should be pure interface #219

Closed
lcy03406 opened this issue Jan 21, 2016 · 4 comments · Fixed by #452
Closed

Serializer and Deserializer should be pure interface #219

lcy03406 opened this issue Jan 21, 2016 · 4 comments · Fixed by #452
Assignees
Milestone

Comments

@lcy03406
Copy link

For now the Serializer and Deserializer traits contain many functions, but many of them have default implementations. A problem is, the default implementations make too many assumption about the serializing protocol, probably based on json. Though them may be convenient for implementing a json serializer, or other text protocols, but most of them do not suit for a binary protocol. The point is, Serializer and Deserializer are so big, that when implementing a concrete serializer/deserializer it is quite possible to accidentally miss some functions. So it is better to remove the default implementations, and make the Serializer and Deserializer traits a 'checklist', the compiler can help checking the implemention. In addition, the current default implementations can be moved to some JsonlikeSerializer or CommonTextSerializer sub-traits.

@oli-obk
Copy link
Member

oli-obk commented Jan 21, 2016

I agree. This "magic" has caused me some headache debugging, so I ended up replacing all of them manually.

@JohnHeitmann
Copy link
Contributor

+1. I think magic layers are more appropriate for Deserialize/Serialize sugar, which are types that many serde users need to interact with. The -er types are relatively rarely implemented, and harder for an author to get complete ambient test coverage of via their own client code, so sacrificing sugar for correctness feels appropriate.

@erickt
Copy link
Member

erickt commented Feb 5, 2016

I can get behind this. One downside though with making this always pure is that any new hint we add would trigger a major release, or we just add a default implementation for that method. Would everyone be okay with this?

@erickt erickt added the RFC label Feb 5, 2016
@jwilm
Copy link
Contributor

jwilm commented Feb 5, 2016

@erickt adding a default implementation when adding a hint seems preferable to a major release. It's entirely backwards compatible this way, and formats which need the hinting can start using it immediately. Since this is all about getting rid of default impls, issues should be added to remove the default impl come the next major version.

@dtolnay dtolnay added this to the v0.8.0 milestone Jul 18, 2016
@dtolnay dtolnay self-assigned this Jul 18, 2016
@dtolnay dtolnay mentioned this issue Jul 18, 2016
4 tasks
@dtolnay dtolnay assigned oli-obk and unassigned dtolnay Jul 18, 2016
rubdos pushed a commit to rubdos/serde that referenced this issue Jun 20, 2017
Inline small functions, especially wrappers

We already had `#[inline]` throughout a lot of the code, but notably some
functions which simply wrap inherent methods were not inlined.  That means
external references will get a full function call, when they could have been
optimized to as little as one opcode.

This PR inlines all functions that look tiny enough for this to matter.

Fixes serde-rs#218.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

6 participants