Skip to content

Add dec!() macro by example #692

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

Merged
merged 7 commits into from
Feb 23, 2025
Merged

Add dec!() macro by example #692

merged 7 commits into from
Feb 23, 2025

Conversation

daniel-pfeiffer
Copy link
Contributor

As discussed, here is a cleaned up, further optimised and integrated new implementation of dec!. It is a completely const and compile-time drop in replacement. This requires Rust 1.79.

Your Result<…String…> is not destructible, i.e. not usable in const. Therefore the new parser has its own Result, that can be transparently converted by ?. This also required splitting try_from_i128_with_scale which thus became simpler.

For now the parser does only exact in i128. Rounding should be addable, if you want to switch parsing completely to this.

Being generic on a smaller type doesn’t currently seem possible in const. Should a benchmark show this to be slower than parsing into u64, the old and new parsers might continue to coexist, till a solution is found.

Copy link
Owner

@paupino paupino left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in getting to this - and thank you for putting this together!

Added a couple of comments, however it really boils down to two requested changes I think:

First is to put this behind a feature flag macros. That way, we can remove some unnecessary ambiguity (also, making this an opt in feature). It means prelude will keep working until people have a chance to migrate over.

The other thing we should do is clean up the old macros folder (i.e. remove it completely). Personally, I think having an opinionated way forward is better - also, since it is a separate crate it should be fine to completely remove (i.e. folks can still use the old crate). Of course, this requires some clean up of the documentation to remove the reference to the crate too.

Thanks again!

src/lib.rs Outdated
@@ -64,7 +64,7 @@ pub use maths::MathematicalOps;
pub mod prelude {
#[cfg(feature = "maths")]
pub use crate::maths::MathematicalOps;
pub use crate::{Decimal, RoundingStrategy};
pub use crate::{dec, Decimal, RoundingStrategy};
Copy link
Owner

Choose a reason for hiding this comment

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

If we feature flag it, we'd just need to optionally include it here.

@daniel-pfeiffer
Copy link
Contributor Author

I have added a feature "macro" (singular, as there is only 1, and to differentiate it from your earlier feature) only for the prelude.
I have removed all mentions and uses of rust_decimal_macros.
I have not removed your old sub-crate. I guess you understand better how that is dealt with.

@paupino paupino merged commit 9689971 into paupino:master Feb 23, 2025
15 checks passed
@paupino
Copy link
Owner

paupino commented Feb 23, 2025

Thank you for your help!

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