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

derive: update dependencies #353

Closed
wants to merge 1 commit into from
Closed

Conversation

hcpl
Copy link

@hcpl hcpl commented Dec 14, 2017

Fixes #352.

Newer versions support new syntax, such as pub(restricted).

Newer versions support new syntax, such as pub(restricted).
@cuviper
Copy link
Member

cuviper commented Dec 14, 2017

warning: unreachable expression
  --> derive/tests/trivial.rs:15:43
   |
15 | #[derive(Debug, PartialEq, FromPrimitive, ToPrimitive)]
   |                                           ^^^^^^^^^^^
   |
   = note: #[warn(unreachable_code)] on by default

error[E0004]: non-exhaustive patterns: type Color is non-empty
  --> derive/tests/trivial.rs:15:43
   |
15 | #[derive(Debug, PartialEq, FromPrimitive, ToPrimitive)]
   |                                           ^^^^^^^^^^^
   |

help: Please ensure that all possible cases are being handled; possibly adding wildcards or more match arms.
  --> derive/tests/trivial.rs:15:43
   |
15 | #[derive(Debug, PartialEq, FromPrimitive, ToPrimitive)]
   |                                           ^^^^^^^^^^^

error: aborting due to previous error

error: Could not compile `num-derive`.

Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

Looks like those errors are just because #(variants,)* should now be #(#variants,)*.

@@ -42,8 +44,8 @@ pub fn from_primitive(input: TokenStream) -> TokenStream {
panic!("`FromPrimitive` can be applied only to unitary enums, {}::{} is either struct or tuple", name, ident)
},
}
if let Some(val) = variant.discriminant {
idx = val.value;
if let Some(Lit(Int(value, _))) = variant.discriminant {
Copy link
Member

Choose a reason for hiding this comment

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

Can we support the full ConstExpr, instead of picking it apart? This will fail on even simple values like -100 which will map to something like Unary(Neg, Lit(Int(100))). Nevermind more complicated expressions. (It seems the old syn just failed to parse it at all.)

I'm thinking instead of idx, we store the actual expr and an offset 0, which we encode as quote!(... #expr + #offset ...). If the next variant has a discriminant, update expr and set offset back to 0, otherwise reuse the former expr with an incremented offset.

@cuviper
Copy link
Member

cuviper commented Dec 19, 2017

Please do move this to the new repo and issue, rust-num/num-derive#2

@cuviper cuviper closed this Dec 19, 2017
bors bot added a commit to rust-num/num-derive that referenced this pull request Jan 22, 2018
3: Update deps and derivation algorithm r=cuviper a=hcpl

Fixes #2.

An updated version of rust-num/num#353 which includes suggestions outlined [here](rust-num/num#353 (review)) and [here](https://github.com/rust-num/num/pull/353/files/76b5b2189f2b45e864e14c38c7856be578125931#r157100221):

- Update `quote` and `syn` to parse new Rust syntax;
- Update `compiletest_rs` to solve Manishearth/compiletest-rs#86;
- Add support for arbitrary constant expressions as enum discriminants;
- Remove the need to `extern crate num` just for deriving.

Some notes:
- `#[derive(FromPrimitive)]` now uses if-else to do its job because non-literal expressions are not allowed for pattern matching.
- I added tests for self-containment of `#[derive]` alongside the code testing derivation functionality to keep the tests small. Would it be better to separate concerns?
- `with_custom_value` should have all three tests like `trivial`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants