Skip to content
This repository has been archived by the owner on May 6, 2020. It is now read-only.

Reimplement derive to work with cfg_attr #71

Merged
merged 2 commits into from
Jun 16, 2016
Merged

Reimplement derive to work with cfg_attr #71

merged 2 commits into from
Jun 16, 2016

Conversation

dtolnay
Copy link
Contributor

@dtolnay dtolnay commented Jun 12, 2016

Fixes serde-rs/serde#148.

The implementation expands cfg_attr and derive in-line during decorator expansion rather than as pre/post-expansion passes. I think this is a simpler and more correct design, but at the expense of being more invasive into the libsyntax code.

@erickt
Copy link

erickt commented Jun 16, 2016

@homu: r+ b57c45d

This seems pretty reasonable to me. As a side note, we ought to actually start implementing tests for these edge cases. I know we get coverage for it by way of the serde tests, but we probably should probably have some tests for things like this here. I'm not quite sure what those tests would look like without needing to implement some complicated macros to drive the testing.

homu added a commit that referenced this pull request Jun 16, 2016
Reimplement derive to work with cfg_attr

Fixes serde-rs/serde#148.

The implementation expands `cfg_attr` and `derive` in-line during decorator expansion rather than as pre/post-expansion passes. I think this is a simpler and more correct design, but at the expense of being more invasive into the libsyntax code.
@homu homu merged commit b57c45d into serde-deprecated:master Jun 16, 2016
@dtolnay dtolnay deleted the cfg branch June 16, 2016 16:43
homu added a commit to serde-rs/serde that referenced this pull request Jun 23, 2016
Add test for expansion inside of cfg_attr

Test for #148. Requires serde-deprecated/syntex#71 + Syntex release.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants