Skip to content
This repository has been archived by the owner on Aug 15, 2021. It is now read-only.

Major rewrite #43

Merged
merged 4 commits into from
Sep 23, 2017
Merged

Major rewrite #43

merged 4 commits into from
Sep 23, 2017

Conversation

sfackler
Copy link
Collaborator

This commit consists of a rewrite of basically all of the serialization
and deserialization logic. The API and internals are based heavily off
of serde_json, in particular, the error type and support for borrowed
deserialization. The deserialization logic is now based off of the tag
byte lookup table described in appendix B of RFC 7049. It's a bit more
verbose, but compiles down to a jump table so there should be some
decent performance benefits in deserialization. It should also resolve
some longstanding correctness issues with deserialization of indefinite
length maps and sequences. The packed encoding can now properly handle
conditionally skipped fields and doesn't need special logic on the
deserialization side due to new serde features.

Closes #37
Closes #36
Closes #32
Closes #31

Copy link
Owner

@pyfisch pyfisch left a comment

Choose a reason for hiding this comment

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

🎉 😄 Thanks for your great work!

I've only found a few minor nitpicks. Is there anything I should test in particular?

Field init shorthands require Rust v1.17+. Please update .travis.yml.

src/de.rs Outdated
0x5c...0x5e => Err(self.error(ErrorCode::UnassignedCode)),
0x5f => self.parse_indefinite_bytes(visitor),

/// Major type 3: a text string
Copy link
Owner

Choose a reason for hiding this comment

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

Use normal comment.

src/de.rs Outdated
self.read.peek().map_err(Error::io)
}

fn eat(&mut self) {
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe fn consume like in BufReader?

src/error.rs Outdated
Error::__Nonexhaustive => unreachable!(),
match self.0.code {
ErrorCode::Io(ref err) => error::Error::description(err),
_ => "JSON error",
Copy link
Owner

Choose a reason for hiding this comment

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

"CBOR error"

@jq-rs
Copy link

jq-rs commented Sep 16, 2017

Looks fantastic! Maybe 1.0 after these changes? If so, maybe we can get rid of unmaintained on the crate description after 1.0 as it really scares everybody?

@pyfisch
Copy link
Owner

pyfisch commented Sep 16, 2017

Maybe 1.0 after these changes?
Issue #3 is still open. I would like to see this fixed but it does not seem to be a priority for serde itself (and might even require a breaking release for the main serde crate). Before 1.0 the crate should also be fuzzed again and used for some time.

If so, maybe we can get rid of unmaintained on the crate description after 1.0 as it really scares everybody?

If I can find a trustworthy person wanting to maintain the crate the unmaintained can be removed. dtolnay rejected the idea to move the crate to the @serde-rs organization so it can't probably become an official serde crate. Maybe @sfackler wants to adopt the crate but he did not say so yet. (If no one is found to adopt the crate we can discuss to make the unmaintained label smaller as most long-standing issues are resolved with this rewrite and I will still review issues and PRs)

@sfackler
Copy link
Collaborator Author

Yeah, I'd be happy to adopt the crate if that'd help with things!

The other big change that I'd want to make before a 1.0 is to overhaul Value to more fully support everything in CBOR. There are some tricks that the toml crate pulled with TOML datetime values that we can hopefully use here to get tags and things like that out in a useable way.

@sfackler
Copy link
Collaborator Author

Updated!

This commit consists of a rewrite of basically all of the serialization
and deserialization logic. The API and internals are based heavily off
of serde_json, in particular, the error type and support for borrowed
deserialization. The deserialization logic is now based off of the tag
byte lookup table described in appendix B of RFC 7049. It's a bit more
verbose, but compiles down to a jump table so there should be some
decent performance benefits in deserialization. It should also resolve
some longstanding correctness issues with deserialization of indefinite
length maps and sequences. The packed encoding can now properly handle
conditionally skipped fields and doesn't need special logic on the
deserialization side due to new serde features.

Closes pyfisch#37
Closes pyfisch#36
Closes pyfisch#32
Closes pyfisch#31
@pyfisch pyfisch merged commit d3ff0a5 into pyfisch:master Sep 23, 2017
@pyfisch
Copy link
Owner

pyfisch commented Sep 23, 2017

Thanks a lot!

@arthurprs
Copy link

arthurprs commented Sep 26, 2017

This is great work.

I think the readme unmaintained alert will throw a lot of people off though, maye remove it?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants