Skip to content

Add BigDecimal #193

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

Closed
wants to merge 2 commits into from
Closed

Add BigDecimal #193

wants to merge 2 commits into from

Conversation

iterion
Copy link

@iterion iterion commented May 17, 2016

This is a totally naive and somewhat broken implementation. I'm new to rust and BigDecimal implementation, but I thought I'd get this out there early for some guidance on ways I can improve my code before getting too deep.
#8 is the second oldest issue on this repo and I hadn't seen any code. So, I figured the work I've done would be useful to future prospective implementors, even if I don't manage to finish.

Add, Sub, and Mul all seem to work correctly (for my single test case each 😄)
Div is currently broken, as it only returns the integer portion instead
of a float as one might expect. Other traits are stubbed out to satisfy
the Num trait.

iterion added 2 commits May 17, 2016 12:13
Div is currently broken, as it only returns the integer portion instead
of a float as one might expect. Other traits are stubbed out to satisfy
the Num trait.
@cuviper
Copy link
Member

cuviper commented May 26, 2016

It seems like a good start! I won't nitpick this too much yet, but I'm glad you're working on it.

A big consideration is what you expose in the public interface. In particular, set_scale sounds like it should modify the object in place, but you're taking &self and returning an adjusted clone. I would also expect this to allow decreasing the scale.

Do you really want the semantics of a negative scale? So I guess something like -2 means the number represents hundreds?

In general, try to avoid clones if you can. For instance in Mul it could multiply the int_val by reference.

@hoodie
Copy link

hoodie commented Sep 8, 2016

Hey there,
is this still on anybody's radar? We could really use a good Decimal implementation 😀

@cuviper
Copy link
Member

cuviper commented Sep 8, 2016

I'm still open to it. Perhaps if @iterion is too busy or no longer interested, someone else could run with it.

@bluss
Copy link
Contributor

bluss commented Sep 8, 2016

There's the option for anyone to publish it as a separate crate/repo first and then discuss inclusion. That might help speed of development in the start.

@iterion
Copy link
Author

iterion commented Sep 8, 2016

I'm still interested, but don't have time right now. So, if someone has time/motivation they should definitely take this over.

@akubera
Copy link

akubera commented Sep 16, 2016

I've extended @iterion's BigDecimal class and added more test cases, and I think things are looking pretty solid: https://github.com/akubera/num/blob/topic/big-decimal/bigdecimal/src/lib.rs.

My first step was to merge the current master with @iterion branch, so the diffs might look like more complex than it is. Here's the simplified diff-to-iterion-fork.

I added an arbitrary precision limit to 100. This is something that should be discussed; should there be a 'precision context' object that may be set during runtime or a compile-time constant which simplifies use? You can see the effect of this magic 100 number at this line of the division operator test.

I tried to make pretty thorough tests (bottom of file). The parser ignores underscores in numbers, and understands exponential form (1e3). Please send me any questions/comments!


I'm not sure what the procedure is to merge a fork of a fork. Let me know if I should create a pull request directly to the @iterion fork. If requested, I could rebase my work before my "master merge" to make the graph a little nicer, or do it as-is.

@cuviper
Copy link
Member

cuviper commented Sep 16, 2016 via email

@akubera akubera mentioned this pull request Sep 16, 2016
@akubera
Copy link

akubera commented Sep 16, 2016

Good news, git keeps author information in a rebase, so @iterion's commits are kept. I rebased his branch with num/master, and cherry-picked all commits after my master-merge; everything should be linear.

This can further be discussed in pull request #224.

Thanks.

@iterion
Copy link
Author

iterion commented Sep 16, 2016

Thanks @akubera! Closing this since we have a replacement in the works.

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.

5 participants