Derive various num traits for newtypes by asayers · Pull Request #17 · rust-num/num-derive

asayers · 2018-09-29T13:14:22Z

This PR adds deriving macros for the following traits:

FromPrimitive
ToPrimitive
NumOps<Self, Self>
NumCast
Zero
One
Num
Float

These macros only work when applied to a newtype where the inner type already
implements the trait in question. They produce the obvious
"unwrap-call-rewrap" implementation.

The FromPrimative and ToPrimative macros now handle both newtypes and the
enums that they used to handle.

The odd one out is NumOps, for two reasons. First, NumOps is just a trait
alias for Add + Sub + Mul + Div + Rem, so those are the traits which the
macro generates impls for. I suppose it's not a great user experience to say
#[derive(Foo)] and end up with impl Bar, but I think in this case it's just
about OK. After all, the user needs only to look at the definition of NumOps
to know that this is only possible behaviour - there's no other way to get an
impl for NumOps.

The other reason this one is strange is that when the user says
#[derive(NumOps)], the macro decides that what they want is an impl for
NumOps<Rhs=Self, Output=Self>. I think this makes sense though; these are
the default type parameters, so when you say NumOps unqualified this is what
it means.

As you can probably tell, my objective here was to get to Float. That's why
this PR only provides macros for a subset of the traits in num_traits. Our
codebase has a bunch of newtyped f64s, and I don't want people unwrapping
them all the time to work with them.

To-do

docs
tests
RELEASES entry

asayers · 2018-09-29T14:24:02Z

Pushed a fix for rust 1.15 compatibility.

asayers · 2018-09-30T03:54:37Z

I just noticed #1. I'm not sure if anything came of the more general derive_ops stuff discussed there - I couldn't find it.

asayers · 2018-09-30T05:27:32Z

Added some docs. Build failure looks spurious.

cuviper

I restarted that failed build -- ✔️

This looks really good! My only concern is how to get proper support for 128-bit integers.

cuviper · 2018-10-02T21:51:37Z


 #![crate_type = "proc-macro"]
 #![doc(html_root_url = "https://docs.rs/num-derive/0.2")]
+#![recursion_limit="512"]


Yeah... It may not need to be this high - I only know that 256 is not enough.

cuviper · 2018-10-02T21:56:18Z

+                    None
+                }
+            }
+            _ => None,


It would be nice if this could also work with a single Fields::Named, but we can leave that as a future extension. I guess we'd have to return an extra token to abstract how to access .0 vs. .inner, and find some way to abstract the newtype constructor too.

I added a specialised error message if the user tries to derive for a named-field-newtype. This is a lazy cop-out on my part, but I think it's better than nothing.

--> tests/newtype.rs:89:5 | 89 | Float, | ^^^^^ | = help: message: num-derive doesn't know how to handle newtypes with named fields yet. Please use a tuple-style newtype, or submit a PR!

cuviper · 2018-10-02T22:08:44Z

+                fn from_i32(n: i32) -> Option<Self> {
+                    <#inner_ty as _num_traits::FromPrimitive>::from_i32(n).map(#name)
+                }
+                #[cfg(has_i128)]


Does this cfg actually work? I'm assuming that this is not interpreted locally, just quoted and emitted as-is to the derived code.

This is how the methods are defined in num-traits, but that has a build script to set the cfg, which only applies within that crate. So I think this derive will be missing out, unless the target crate also happens to set has_i128.

I think what we could do is add a similar build script to num-derive, and check #[cfg(has_i128)] locally (outside of quote!) to determine whether we should emit those methods in the derived output.

(The enum derivation doesn't worry about this yet, since repr128 is still unstable.)

I see; you're saying that, in the end, these functions are unconditionally ignored. I'll implement your suggestion.

Done; I confirmed that the functions are present in the expansion under 1.29, and it still builds with 1.15.

cuviper · 2018-10-02T22:10:49Z

    };

-    res.into()
+    dummy_const_trick("FromPrimative", &name, impl_).into()


spelling: FromPrimitive
(though I know this is hidden anyway)

Thanks, fixed.

This equates to deriving Add, Sub, Mul, Div, and Rem, where RHS=Self and Output=Self.

This is needed for older compilers, even though we know they won't have i128 support anyway...

cuviper

Looks great! I just added explicit tests for the 128-bit methods.

cuviper · 2018-10-03T17:05:10Z

bors r+

17: Derive various num traits for newtypes r=cuviper a=asayers This PR adds deriving macros for the following traits: * FromPrimitive * ToPrimitive * NumOps<Self, Self> * NumCast * Zero * One * Num * Float These macros only work when applied to a newtype where the inner type already implements the trait in question. They produce the obvious "unwrap-call-rewrap" implementation. The `FromPrimative` and `ToPrimative` macros now handle both newtypes and the enums that they used to handle. The odd one out is `NumOps`, for two reasons. First, `NumOps` is just a trait alias for `Add + Sub + Mul + Div + Rem`, so those are the traits which the macro generates impls for. I suppose it's not a great user experience to say `#[derive(Foo)]` and end up with `impl Bar`, but I think in this case it's just about OK. After all, the user needs only to look at the definition of `NumOps` to know that this is only possible behaviour - there's no other way to get an impl for `NumOps`. The other reason this one is strange is that when the user says `#[derive(NumOps)]`, the macro decides that what they want is an impl for `NumOps<Rhs=Self, Output=Self>`. I think this makes sense though; these are the default type parameters, so when you say `NumOps` unqualified this is what it means. As you can probably tell, my objective here was to get to `Float`. That's why this PR only provides macros for a subset of the traits in `num_traits`. Our codebase has a bunch of newtyped `f64`s, and I don't want people unwrapping them all the time to work with them. ### To-do - [x] docs - [x] tests - [x] RELEASES entry Co-authored-by: Alex Sayers <alex.sayers@gmail.com> Co-authored-by: Josh Stone <cuviper@gmail.com>

bors · 2018-10-03T17:10:54Z

Build succeeded

continuous-integration/travis-ci/push

asayers · 2018-10-03T18:16:11Z

Thanks @cuviper!

asayers force-pushed the newtypes branch from 7693788 to ea052f2 Compare September 29, 2018 14:23

asayers force-pushed the newtypes branch 2 times, most recently from 079f045 to b64a25e Compare September 29, 2018 17:05

Factor out dummy_const_trick()

130c9fd

asayers force-pushed the newtypes branch 2 times, most recently from fd55292 to a69f860 Compare September 30, 2018 04:54

cuviper reviewed Oct 2, 2018

View reviewed changes

asayers added 8 commits October 3, 2018 16:29

Add the standard num-* build.rs file

9a9c80c

Derive {To,From}Primitive for newtypes

75b8972

Derive NumOps<Self, Self> for newtypes

52e86e2

This equates to deriving Add, Sub, Mul, Div, and Rem, where RHS=Self and Output=Self.

Derive NumCast for newtypes

e4d40fe

Derive Zero and One for newtypes

b099900

Derive Num for newtypes

a2887ac

Derive Float for newtypes

2e751bf

Add "newtype deriving" entry to RELEASES

8f54ad2

asayers force-pushed the newtypes branch from a69f860 to 8f54ad2 Compare October 3, 2018 09:31

cuviper added 2 commits October 3, 2018 09:58

Explicitly use build.rs in Cargo.toml

203a3db

This is needed for older compilers, even though we know they won't have i128 support anyway...

Add explicit tests of has_i128-derived methods

706ea88

cuviper approved these changes Oct 3, 2018

View reviewed changes

bors Bot merged commit 706ea88 into rust-num:master Oct 3, 2018

cuviper mentioned this pull request Nov 8, 2018

Clippy error "useless lint attribute" when using FromPrimitive #20

Closed

Conversation

asayers commented Sep 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To-do

Uh oh!

asayers commented Sep 29, 2018

Uh oh!

asayers commented Sep 30, 2018

Uh oh!

asayers commented Sep 30, 2018

Uh oh!

cuviper 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cuviper left a comment

Choose a reason for hiding this comment

Uh oh!

cuviper commented Oct 3, 2018

Uh oh!

bors Bot commented Oct 3, 2018

Build succeeded

Uh oh!

asayers commented Oct 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

asayers commented Sep 29, 2018 •

edited

Loading