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

Update deps and derivation algorithm #3

Merged
merged 6 commits into from
Jan 22, 2018

Conversation

hcpl
Copy link
Contributor

@hcpl hcpl commented Dec 25, 2017

Fixes #2.

An updated version of rust-num/num#353 which includes suggestions outlined here and here:

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.

hcpl added 2 commits December 25, 2017 15:48
- 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.
This was made to bypass wrong type inference, such as `-10 as u64`.
@hcpl
Copy link
Contributor Author

hcpl commented Dec 25, 2017

compiletest_rs fails in current nightly: FnBox passed to TestFn::DynTestFn now takes no arguments.

Upstream PR to fix that: Manishearth/compiletest-rs#93.

@hcpl hcpl changed the title Update deps derive algorithm Update deps and derivation algorithm Dec 25, 2017
@cuviper
Copy link
Member

cuviper commented Jan 8, 2018

syn 0.12 and quote 0.4 were just released, which appear to be significant refactors.
https://users.rust-lang.org/t/syn-0-12-complete-redesign-for-all-your-macros-1-1-and-2-0-needs/14917

hcpl added 2 commits January 19, 2018 17:15
Will likely be able to parse even more new Rust syntax.

This transition required some API-incompatible changes.
@hcpl
Copy link
Contributor Author

hcpl commented Jan 19, 2018

@cuviper thanks! I've adapted the code to use new syn and quote versions.

I have a concern about using the "full" feature for syn though: it is now required to parse the Color enum from with_custom_values.rs (code breakage during syn 0.11 -> 0.12 migration with default features is discussed here), but increases the compilation time. Is being able to parse more complex expressions as enum values worth it?

@cuviper
Copy link
Member

cuviper commented Jan 19, 2018

I have a concern about using the "full" feature for syn though: it is now required [...], but increases the compilation time.

Can you give me some numbers?

Is being able to parse more complex expressions as enum values worth it?

Unless it's extreme, then yes, I would lean in favor of functionality and accept the compilation time.

@dtolnay
Copy link

dtolnay commented Jan 19, 2018

The "full" feature should not be enabled in num-derive because nothing in num-derive depends on it. If a downstream library wants to use Alpha = (-3 - (-5isize)) - 10 they can enable "full" and have it work without num-derive knowing about that.

Ordinarily I would expect libraries that are not already using "full" for other reasons will factor out the expression into a constant instead of paying in compile time.

+ const ALPHA: isize = (-3 - (-5isize)) - 10;

+ #[derive(ToPrimitive)]
  enum Color {
      Red,
      Blue = 5,
      Green,
-     Alpha = (-3 - (-5isize)) - 10,
+     Alpha = ALPHA,
   }

@cuviper
Copy link
Member

cuviper commented Jan 19, 2018

@dtolnay

If a downstream library wants to use Alpha = (-3 - (-5isize)) - 10 they can enable "full" and have it work without num-derive knowing about that.

Wouldn't that suggestion make syn effectively a public dependency? If we later update to syn 0.13, such a downstream library would be broken until they update in kind.

I suppose num-derive could have its own feature which enables syn/full though.

@dtolnay
Copy link

dtolnay commented Jan 19, 2018

Yeah you're right. An optional feature would work.

@hcpl
Copy link
Contributor Author

hcpl commented Jan 20, 2018

@dtolnay

Can you give me some numbers?

In num-derive working directory:

  • Without "full":

    $ time cargo build
       Compiling syn v0.12.7
       Compiling num-derive v0.1.41 (...)
        Finished dev [unoptimized + debuginfo] target(s) in 19.91 secs
    
    real	0m20.513s
    user	0m34.116s
    sys	0m1.532s
    
  • With "full":

    $ time cargo build
       Compiling syn v0.12.7
       Compiling num-derive v0.1.41 (...)
        Finished dev [unoptimized + debuginfo] target(s) in 36.57 secs
    
    real	0m37.172s
    user	0m59.300s
    sys	0m2.200s
    

I suppose num-derive could have its own feature which enables syn/full though.

Good idea. I'll implement it under "full-syntax" name which is short and descriptive enough to me.

hcpl added 2 commits January 20, 2018 16:41
I couldn't find a way to use "full-syntax" only for `with_custom_values`
:(.
@cuviper
Copy link
Member

cuviper commented Jan 22, 2018

Thanks, looks great!

bors r+

bors bot added a commit 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`.
@bors
Copy link
Contributor

bors bot commented Jan 22, 2018

Build succeeded

@bors bors bot merged commit 0f58c1c into rust-num:master Jan 22, 2018

[dev-dependencies.num]
version = "0.1"

[dev-dependencies.num-derive]
path = "."
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, actually, cargo package doesn't allow any path declarations like this.

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.

3 participants