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 support for serde #43

Merged
merged 7 commits into from Aug 8, 2015
Merged

Add support for serde #43

merged 7 commits into from Aug 8, 2015

Conversation

@erickt
Copy link
Contributor

erickt commented Jul 30, 2015

This is an initial PR that adds support for the yet-to-be-released serde version 0.5.0, so please don't merge this in until that lands. I wanted to submit it to get your input on the design and if you see if there are any gaps in the implementation.

@TyOverby
Copy link
Collaborator

TyOverby commented Jul 30, 2015

Wow! This will take me a while to read through, but it looks great! The one thing that I noticed right off the bat is that we could probably unify most of the unit tests. Instead of having test and test_serde, we could just have test, with proxy functions that use both implementations. Looks like you've already done this actually.

For example

fn proxy_encoded_size<T: ...>(obj: &T) -> usize {
    let ser_size = bincode::encoded_size(obj);
    let serde_size = bincode::serialized_size(obj);
    assert_eq(ser_size, serde_size);
    return ser_size;
}

I noticed that you already did that for the the_same function where you compare the existing implementation with the serde implementation.

I might hold off on reading the rest of the code until I can build and run it myself. At the very least I'll get a refresher on Serde tonight.

@TyOverby
Copy link
Collaborator

TyOverby commented Jul 30, 2015

I am worried about how both of the implementations will be exposed though.

encode and to_vec are identical in terms of functionality, and so is decode and from_slice.

While they should be interchangeable, I think it'll be confusing to library users.

@pcwalton
Copy link

pcwalton commented Aug 4, 2015

@TyOverby Ping. Servo really wants this. Is there a way I can help get this landed?

@TyOverby
Copy link
Collaborator

TyOverby commented Aug 4, 2015

I won't be able to merge this until serde hits 0.5.0 on crates.io, but I will review the code later tonight. It would be great if using serde also sidestepped #41!

@TyOverby
Copy link
Collaborator

TyOverby commented Aug 4, 2015

@erickt: I downloaded your patch and wired it up the path for Serde to your github repo so I could check it with 0.5.0, but there are a set of errors that indicate that serde can't be used with rust-1.0. I thought this was not the case?

$ cargo build
   Compiling aster v0.4.0
   Compiling quasi v0.3.1
   Compiling advapi32-sys v0.1.2
c:/Users/Ty\.cargo\registry\src\github.com-1ecc6299db9ec823\aster-0.4.0\src\item.rs:233:24: 233:48 error: failed to resolve. Could not find `Constness` in `syntax::ast`
c:/Users/Ty\.cargo\registry\src\github.com-1ecc6299db9ec823\aster-0.4.0\src\item.rs:233             constness: ast::Constness::NotConst,
                                                                                                               ^~~~~~~~~~~~~~~~~~~~~~~~
c:/Users/Ty\.cargo\registry\src\github.com-1ecc6299db9ec823\aster-0.4.0\src\item.rs:233:24: 233:48 error: unresolved name `ast::Constness::NotConst`
c:/Users/Ty\.cargo\registry\src\github.com-1ecc6299db9ec823\aster-0.4.0\src\item.rs:233             constness: ast::Constness::NotConst,
                                                                                                               ^~~~~~~~~~~~~~~~~~~~~~~~
c:/Users/Ty\.cargo\registry\src\github.com-1ecc6299db9ec823\aster-0.4.0\src\item.rs:247:16: 247:30 error: use of undeclared type name `ast::Constness`
c:/Users/Ty\.cargo\registry\src\github.com-1ecc6299db9ec823\aster-0.4.0\src\item.rs:247     constness: ast::Constness,
                                                                                                       ^~~~~~~~~~~~~~
c:/Users/Ty\.cargo\registry\src\github.com-1ecc6299db9ec823\aster-0.4.0\src\item.rs:261:26: 261:47 error: failed to resolve. Could not find `Constness` in `syntax::ast`
c:/Users/Ty\.cargo\registry\src\github.com-1ecc6299db9ec823\aster-0.4.0\src\item.rs:261         self.constness = ast::Constness::Const;
                                                                                                                 ^~~~~~~~~~~~~~~~~~~~~
c:/Users/Ty\.cargo\registry\src\github.com-1ecc6299db9ec823\aster-0.4.0\src\item.rs:261:26: 261:47 error: unresolved name `ast::Constness::Const`
c:/Users/Ty\.cargo\registry\src\github.com-1ecc6299db9ec823\aster-0.4.0\src\item.rs:261         self.constness = ast::Constness::Const;
                                                                                                                 ^~~~~~~~~~~~~~~~~~~~~
c:/Users/Ty\.cargo\registry\src\github.com-1ecc6299db9ec823\aster-0.4.0\src\method.rs:28:16: 28:30 error: use of undeclared type name `ast::Constness`
c:/Users/Ty\.cargo\registry\src\github.com-1ecc6299db9ec823\aster-0.4.0\src\method.rs:28     constness: ast::Constness,
                                                                                                        ^~~~~~~~~~~~~~
c:/Users/Ty\.cargo\registry\src\github.com-1ecc6299db9ec823\aster-0.4.0\src\method.rs:50:24: 50:48 error: failed to resolve. Could not find `Constness` in `syntax::ast`
c:/Users/Ty\.cargo\registry\src\github.com-1ecc6299db9ec823\aster-0.4.0\src\method.rs:50             constness: ast::Constness::NotConst,
                                                                                                                ^~~~~~~~~~~~~~~~~~~~~~~~
c:/Users/Ty\.cargo\registry\src\github.com-1ecc6299db9ec823\aster-0.4.0\src\method.rs:50:24: 50:48 error: unresolved name `ast::Constness::NotConst`
c:/Users/Ty\.cargo\registry\src\github.com-1ecc6299db9ec823\aster-0.4.0\src\method.rs:50             constness: ast::Constness::NotConst,
                                                                                                                ^~~~~~~~~~~~~~~~~~~~~~~~
c:/Users/Ty\.cargo\registry\src\github.com-1ecc6299db9ec823\aster-0.4.0\src\method.rs:72:26: 72:47 error: failed to resolve. Could not find `Constness` in `syntax::ast`
c:/Users/Ty\.cargo\registry\src\github.com-1ecc6299db9ec823\aster-0.4.0\src\method.rs:72         self.constness = ast::Constness::Const;
                                                                                                                  ^~~~~~~~~~~~~~~~~~~~~
c:/Users/Ty\.cargo\registry\src\github.com-1ecc6299db9ec823\aster-0.4.0\src\method.rs:72:26: 72:47 error: unresolved name `ast::Constness::Const`
c:/Users/Ty\.cargo\registry\src\github.com-1ecc6299db9ec823\aster-0.4.0\src\method.rs:72         self.constness = ast::Constness::Const;
                                                                                                                  ^~~~~~~~~~~~~~~~~~~~~
error: aborting due to 10 previous errors
Build failed, waiting for other jobs to finish...
c:/Users/Ty\.cargo\registry\src\github.com-1ecc6299db9ec823\quasi-0.3.1\src\lib.rs:11:43: 11:66 error: unstable feature
c:/Users/Ty\.cargo\registry\src\github.com-1ecc6299db9ec823\quasi-0.3.1\src\lib.rs:11 #![cfg_attr(not(feature = "with-syntex"), feature(rustc_private))]
                                                                                                                                ^~~~~~~~~~~~~~~~~~~~~~~
note: this feature may not be used in the stable release channel
error: aborting due to previous error
Could not compile `aster`.

To learn more, run the command again with --verbose.

{
let len = match visitor.len() {
Some(len) => len,
None => panic!("do not know how to serialize a sequence with no length"),

This comment has been minimized.

@TyOverby

TyOverby Aug 4, 2015

Collaborator

Should this be an error inside SerializeError?

}

#[inline]
fn visit_u8<V>(&mut self, mut visitor: V) -> DeserializeResult<V::Value>

This comment has been minimized.

@TyOverby

TyOverby Aug 5, 2015

Collaborator

should this be impl_nums too?

@pcwalton
Copy link

pcwalton commented Aug 7, 2015

@erickt Ping. If you don't have time for this, would you prefer that I pick up the patch and push it through?

@erickt
Copy link
Contributor Author

erickt commented Aug 7, 2015

@pcwalton: Just finally released 0.5.0 and updated this PR to build with it!

@TyOverby
Copy link
Collaborator

TyOverby commented Aug 7, 2015

@erickt @pcwalton: Is this going to build with stable release? I thought the syntex crate was supposed to build on stable?

@TyOverby
Copy link
Collaborator

TyOverby commented Aug 7, 2015

Ok, I got it building with stable (testing requires nightly, but that's acceptable).

There's a few changes that I'd like to make though, so I'll take this PR from here.

@TyOverby TyOverby merged commit 30cb43a into servo:master Aug 8, 2015
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@TyOverby
Copy link
Collaborator

TyOverby commented Aug 8, 2015

Yay merged! 🎉

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.