Skip to content

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,)*.

}
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.

2 participants