Skip to content

Conversation

@dcrewi
Copy link
Contributor

@dcrewi dcrewi commented Sep 6, 2013

No description provided.

@huonw
Copy link
Contributor

huonw commented Sep 6, 2013

Looks pretty good, but could this just be a single trait: RandBigInt { fn gen_bigint(); fn gen_biguint(); }? Also, it should be implemented as impl<R: Rng> RandBigInt for R, not R: RngUtil.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this means that the probability of zero is double that of any other number; it would be good to make sure that it is actually uniform.

@dcrewi
Copy link
Contributor Author

dcrewi commented Sep 6, 2013

Well it could be implemented as a single trait, but is there a reason to? Everything else about the two types is so nicely segregated that they could be moved into separate modules. It seems more consistent to me to maintain the split.

- use identifiers with underscores to avoid unused variable warning
- implement on R: Rng instead of on R: RngUtil
- bugfix: zero BigInts were being generated twice as often as any
  other number
- test that gen_biguint(0) always returns zero
@alexcrichton
Copy link
Member

fwiw, I'm in favor of the traits getting merged. I'm also generally not a fan of having lots and lots of traits, but that's more of just a general guideline that I try to follow.

In this specific case I don't think that it really makes a lot of sense to have one trait per type when it actually fits pretty well to encompass both types at the same time. Things may be separate now between Uint and Int, but I'm always a fan of unifying if possible for clarity and reduction of surface area. Just my opinions though :)

@bors bors closed this Sep 11, 2013
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 16, 2022
Rustup

r? `@ghost`

changelog: none
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.

4 participants