No more syntex for serde_derive by dtolnay · Pull Request #548 · serde-rs/serde

dtolnay · 2016-09-12T03:02:07Z

Follow-up tasks:

factor the attribute stripping into a fold (Factor attribute-stripping into a fold, similar to libsyntax #561)
clean up workarounds (Clean up workarounds from syn conversion #562)

dtolnay · 2016-09-12T07:52:05Z

Running cargo test in serde_derive now passes! It didn't before due to rust-lang/rust#36211.

dtolnay · 2016-09-12T08:05:26Z

For some reason serde_codegen by itself builds three times as fast this way, which I didn't expect. I guess quasi_macros is slow?

$ git checkout master
$ cargo build --no-default-features --features unstable
$ cargo clean -p serde_codegen
$ cargo build --no-default-features --features unstable  # 12 SECONDS
$ git checkout syn
$ cargo build --no-default-features --features with-syn
$ cargo clean -p serde_codegen
$ cargo build --no-default-features --features with-syn  # 4 SECONDS

oli-obk · 2016-09-12T08:14:38Z

I guess quasi_macros is slow?

fewer generics?

dtolnay · 2016-09-13T05:28:12Z

I did some testing with rusoto in rusoto/rusoto#364 (comment) to see how performance of this PR compares with the current serde_macros (runtime performance, after they have been compiled). They come out about even which is great.

Conflicts: serde_codegen/Cargo.toml serde_codegen_internals/Cargo.toml serde_derive/Cargo.toml

Conflicts: serde_codegen/src/ser.rs

dtolnay · 2016-09-27T08:29:32Z

There is a lot of work left here but let's take a moment to celebrate 9a86e68 the first syntex-free green build.

Conflicts: serde_codegen/Cargo.toml

dtolnay · 2016-09-27T17:05:38Z

@oli-obk @erickt this is ready for review. There were a few places that I uncovered strange behavior in the old codegen (just unusually placed semicolons / parentheses / braces) that I went out of my way to preserve. I left comments and I can clean that up in a separate PR. I confirmed that this PR expands the test suite byte-for-byte identical to master. I filed #562 to follow up.

I filed #561 as another follow-up task. I don't think that needs to block this PR.

alexcrichton · 2016-09-27T23:27:39Z

Awesome progress @dtolnay!

oli-obk

This is totally great. I have no complaints that should be seen as blockers. Let's do this!

oli-obk · 2016-09-28T11:09:47Z

serde_codegen/src/de.rs

+            Some(quote!(::<#(ty_param_idents),*>))
+        };
+
+        let phantom_exprs = (0 .. num_phantoms).map(|_| quote!(::std::marker::PhantomData));


std::iter::repeat(quote!(::std::marker::PhantomData)).take(num_phantoms) runs the expanded quote code only once. Not really relevant though.

oli-obk · 2016-09-28T11:10:35Z

serde_codegen/src/de.rs

+                struct __Visitor #generics ( #(phantom_types),* ) #where_clause;
+            },
+            quote!(__Visitor <#(all_params),*> ),
+            quote!(__Visitor #ty_param_idents ( #(phantom_exprs),* )),


❤️ this expansion magic

oli-obk · 2016-09-28T11:29:16Z

serde_codegen/src/lib.rs

-extern crate aster;
-extern crate quasi;
+// The `quote!` macro requires deep recursion.
+#![recursion_limit = "192"]


I'm through with serde_codegen. Big 👍 on the quote! call readability.

I agree. I love how've much simpler everything looks.

oli-obk · 2016-09-28T12:45:03Z

serde_codegen_internals/src/ctxt.rs

+use std::cell::RefCell;
+
+#[derive(Default)]
+pub struct Ctxt {


I'm not very happy with the Ctxt structure and it's usage. I'd much rather have regular Result type error reporting, but we can leave that to a future refactoring. Maybe the code gets horribly cluttered with Result

I filed #563 to follow up in a future refactoring.

erickt

This wasn't amazing. Great work.

erickt · 2016-09-28T13:32:15Z

serde_codegen/src/lib.rs

-extern crate aster;
-extern crate quasi;
+// The `quote!` macro requires deep recursion.
+#![recursion_limit = "192"]


I agree. I love how've much simpler everything looks.

erickt · 2016-09-28T13:37:21Z

serde_codegen/src/ser.rs

-                $type_ident::$variant_ident(ref __simple_value) => $block
-            )
+            quote! {
+                // The braces are unnecessary but quasi used to put them in. We


If I recall correctly, quote_block required the extra braces, so shouldn't be a problem to remove them later.

Conflicts: serde_macros/tests/compile-fail/reject-unknown-attributes.rs

No more syntex for serde_derive

c34baa1

dtolnay added the wip label Sep 12, 2016

Same for deserialization

55e5f19

dtolnay force-pushed the syn branch from 4f2aa8d to 55e5f19 Compare September 12, 2016 07:50

Recursion limit

a69b82c

This was referenced Sep 13, 2016

Remove dependency on Syntex #540

Closed

Excessive CI compile times rusoto/rusoto#364

Closed

dtolnay added 4 commits September 24, 2016 09:29

Merge branch origin/master into origin/syn

51f9557

Conflicts: serde_codegen/Cargo.toml serde_codegen_internals/Cargo.toml serde_derive/Cargo.toml

Bump syn dependency

1eca776

Merge branch origin/master into origin/syn

6cbf0d3

Conflicts: serde_codegen/src/ser.rs

Fix conflicts with the __serde_state commit

8e865f6

dtolnay mentioned this pull request Sep 27, 2016

Decide whether to version codegen/macros separately from serde #376

Closed

dtolnay added 8 commits September 26, 2016 23:17

Remove with-libsyntax feature

8e77960

Mark all serde attributes as used

8ee8c07

Include unknown attribute name in error message

4ad6c4f

Revamp serde_codegen_internals error handling

0c18c15

Update tests to new location of errors

40b8742

Handle various attribute parsing error cases

7e441e5

Fix clippy lints in serde_codegen

7d09b14

Use push_str to support old compilers

9a86e68

dtolnay mentioned this pull request Sep 27, 2016

Fix num-macros FromPrimitive implementation rust-num/num#228

Merged

7 tasks

dtolnay added 2 commits September 27, 2016 09:56

Fix differences in the generated code

effa298

Merge branch origin/master into origin/syn

3a3777a

Conflicts: serde_codegen/Cargo.toml

dtolnay removed the wip label Sep 27, 2016

dtolnay assigned oli-obk Sep 27, 2016

dtolnay mentioned this pull request Sep 27, 2016

Clean up workarounds from syn conversion #562

Closed

dtolnay assigned erickt Sep 27, 2016

This was referenced Sep 27, 2016

Document how to use serde_derive serde-rs/serde-rs.github.io#21

Closed

Tracking issue for "Macros 1.1" (RFC #1681) rust-lang/rust#35900

Closed

Reordering of attributes passed to rustc_macro_derive rust-lang/rust#36211

Closed

oli-obk approved these changes Sep 28, 2016

View reviewed changes

erickt approved these changes Sep 28, 2016

View reviewed changes

dtolnay added 2 commits September 28, 2016 08:57

Use iter::repeat to build phantom exprs

9fe1676

Merge branch origin/master into origin/syn

b0eee50

Conflicts: serde_macros/tests/compile-fail/reject-unknown-attributes.rs

dtolnay merged commit 49d24a1 into master Sep 28, 2016

dtolnay deleted the syn branch September 28, 2016 16:28

dtolnay mentioned this pull request Sep 28, 2016

Clean up syn workarounds #564

Merged

Uh oh!

Conversation

dtolnay commented Sep 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dtolnay commented Sep 12, 2016

Uh oh!

dtolnay commented Sep 12, 2016

Uh oh!

oli-obk commented Sep 12, 2016

Uh oh!

dtolnay commented Sep 13, 2016

Uh oh!

dtolnay commented Sep 27, 2016

Uh oh!

dtolnay commented Sep 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexcrichton commented Sep 27, 2016

Uh oh!

oli-obk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erickt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

dtolnay commented Sep 12, 2016 •

edited

Loading

dtolnay commented Sep 27, 2016 •

edited

Loading