-
Notifications
You must be signed in to change notification settings - Fork 27
Constant d128 macros #16
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
Conversation
Update with latest changes from master
|
Thanks for the contribution @trolleyman. Give me a week to carve some time to take a closer look. |
| } | ||
| }; | ||
|
|
||
| match &ex.node { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this ampersand?
|
Thanks for the contribution! In addition to the comments above: don't we need some tests? |
| extern crate decimal; | ||
|
|
||
| fn main() { | ||
| let a = d128!(0.1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about better tests?
d128!(0.1) should be equal to 1 as dec128 / 10 as dec128, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, I should - was just kinda rushing to get it out the door and working :p I had so much trouble with it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose this code because it fails in f32 land: http://0.30000000000000004.com/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this fail?
d128!(0.1) == (1 as dec128) / (10 as dec128)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test with this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is different. Let me explain. What you are testing here is that the result of d128!(0.1) is equal to the result of d128!(1) / d128!(10). This might pass even if d128!(10) does not equal a dec128 of decimal value 10. This is why I suggest that the left hand side uses the macro and the right hand side uses conversion from int to dec128 which are trivial.
Also is this test organization going to work with cargo test? If not can you put these tests inside decimal-macros/src/lib.rs inside a test module and annotate appropriately so that it works?
|
Added some commits that polish up the PR a bit |
|
|
||
| fn d128_lit<'cx>(cx: &'cx mut ExtCtxt, sp: Span, tts: &[TokenTree]) -> Box<MacResult + 'cx> { | ||
| // Turn input into a string literal | ||
| // e.g. d128!(0.1) -> "0.1", d128!(NaN) -> "NaN" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this comment. It makes it easier for the syntex noob (me) out there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
decimal-macros/src/lib.rs
Outdated
| let ids = vec![cx.ident_of("decimal"), cx.ident_of("d128"), cx.ident_of("from_bytes")]; | ||
| let ex = cx.expr_call_global(lit.span, ids, vec![vec]); | ||
|
|
||
| let ex = cx.expr_call_global(lit.span, ids, vec![vec]); // Call ::decimal::d128::from_bytes with array literal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is unnecessary (the expr_call_global is pretty obvious).
|
Ok, updated |
| #[doc(hidden)] | ||
| pub fn from_bytes(bytes: [u8; 16]) -> d128 { | ||
| d128 { | ||
| bytes: unsafe { ::std::mem::transmute(bytes) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work in Big Endian machines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will, as it's just calling the same code that from_str calls.
|
It looks like with the new Macros 1.1 this should be even easier. Do you want to take a stab at it? |
This makes macro invocations free at runtime, whereas the old macro delegated to from_str at runtime which has small but nonzero cost. The concept is based on alkis#16 but completely rewritten with the newer (stable) procedural macros which implies a requirement for rustc 1.31.0 (2018 edition). The implementation uses the 2021 edition (rustc 1.56.0) for convenience.
This makes macro invocations free at runtime, whereas the old macro delegated to from_str at runtime which has small but nonzero cost. The concept is based on alkis#16 but completely rewritten with the newer (stable) procedural macros which implies a requirement for rustc 1.31.0 (2018 edition). The implementation uses the 2021 edition (rustc 1.56.0) for convenience.
|
Closing due to #72 |
This makes macro invocations free at runtime, whereas the old macro delegated to from_str at runtime which has small but nonzero cost. The old decimal::d128 macro is changed to be internal-only so the new one doesn't have a conflicting name: the type and macro share a name so it's not possible to import only one of them from a given crate. The concept is based on alkis#16 but completely rewritten with the newer (stable) procedural macros which implies a requirement for rustc 1.31.0 (2018 edition). The implementation uses the 2021 edition (rustc 1.56.0) for convenience.
Fixes #7.
A new crate is located in the decimal-macros folder that provides the constant macro. It uses a plugin to compile the d128 literals.