Skip to content
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

Add iter_u32_digits , iter_u64_digits, to_u64_digits to both BigInt and BigUint #158

Closed
wants to merge 14 commits into from
Closed

Conversation

Speedy37
Copy link
Contributor

@Speedy37 Speedy37 commented Jun 28, 2020

This adds: iter_u32_digits , iter_u64_digits, to_u64_digits to both BigInt and BigUint

@Speedy37 Speedy37 marked this pull request as ready for review June 28, 2020 00:39
@Speedy37 Speedy37 changed the title Add iter_u32_digits and iter_u64_digits to biguint Add iter_u32_digits , iter_u64_digits, to_u64_digits to both BigInt and BigUint Jun 28, 2020
@Speedy37
Copy link
Contributor Author

I'm wondering if adding iter_u32_digits/iter_u64_digits to BigInt is a good idea.
I guess users can use (magnitude().iter_u32_digits()) if they needs too.

Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the PR, and sorry for the late review.

I'm wondering if adding iter_u32_digits/iter_u64_digits to BigInt is a good idea.

I think it's fine to keep API parity between BigInt and BigUint for most things.

src/bigint.rs Outdated Show resolved Hide resolved
src/biguint.rs Outdated Show resolved Hide resolved
src/biguint.rs Outdated Show resolved Hide resolved
src/biguint.rs Outdated Show resolved Hide resolved
src/biguint.rs Outdated Show resolved Hide resolved
src/biguint.rs Outdated Show resolved Hide resolved
src/biguint.rs Outdated Show resolved Hide resolved
src/bigint.rs Outdated Show resolved Hide resolved
src/bigint.rs Outdated Show resolved Hide resolved
@Speedy37
Copy link
Contributor Author

Should iter_u32_digits and iter_u64_digits on BigInt also returns the (Sign, Iterator) couple to match other similar method return types?

@Speedy37 Speedy37 requested a review from cuviper August 19, 2020 01:37
@cuviper
Copy link
Member

cuviper commented Aug 20, 2020

I think it's OK that these methods return just the iterator, because that makes chaining easier, and fn sign can be used separately.

src/bigint.rs Outdated Show resolved Hide resolved
@Speedy37
Copy link
Contributor Author

I fixed the latest set of requested changes.

@Speedy37 Speedy37 requested a review from cuviper August 20, 2020 01:01
@cuviper
Copy link
Member

cuviper commented Aug 21, 2020

Can you add #[inline] on all of the iterator methods? I tried myself, and it makes a huge difference to the benchmarks:

 name             base.x86_64 ns/iter  inline.x86_64 ns/iter  diff ns/iter   diff %  speedup
 iter_u32_digits  130                  62                              -68  -52.31%   x 2.10
 iter_u64_digits  64                   8                               -56  -87.50%   x 8.00
 to_u32_digits    152                  78                              -74  -48.68%   x 1.95
 to_u64_digits    85                   32                              -53  -62.35%   x 2.66
 name             base.i686 ns/iter  inline.i686 ns/iter  diff ns/iter   diff %  speedup
 iter_u32_digits  136                14                           -122  -89.71%   x 9.71
 iter_u64_digits  111                42                            -69  -62.16%   x 2.64
 to_u32_digits    154                62                            -92  -59.74%   x 2.48
 to_u64_digits    127                68                            -59  -46.46%   x 1.87

(I was going to just do it, but it seems your branch isn't open to maintainer pushes.)

@Speedy37
Copy link
Contributor Author

Done

Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

One more bikesheddy thought -- maybe we should drop the iter_/Iter prefixes? e.g. u32_digits -> U32Digits. I was considering the str iterators for prior art with bytes() -> Bytes and chars() -> Chars, or the maps have keys -> Keys, etc. I think it's uncommon to use iter/Iter in the names unless that's all there is to it (Iter/IterMut/IntoIter).

@@ -2255,6 +2266,230 @@ pub(crate) fn to_str_radix_reversed(u: &BigUint, radix: u32) -> Vec<u8> {
res
}

/// An iterator of `u32` digits representation of the `BigUint` ordered least
/// significant digit first.
Copy link
Member

Choose a reason for hiding this comment

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

I suggest:

/// An iterator of the `u32` digits representation of a `BigUint` or `BigInt`,
/// ordered least significant digit first.

Also, we should reexport this struct in the root, because this module is not public.

(Ditto for IterU64Digits.)

impl ExactSizeIterator for IterU64Digits<'_> {
#[inline]
fn len(&self) -> usize {
self.it.len()
Copy link
Member

Choose a reason for hiding this comment

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

nit: this is identical to the cfg(u64_digit) version, so we could just have a unified version.

/// assert_eq!(BigUint::from(1125u32).to_u64_digits(), vec![1125]);
/// assert_eq!(BigUint::from(4294967295u32).to_u64_digits(), vec![4294967295]);
/// assert_eq!(BigUint::from(4294967296u64).to_u64_digits(), vec![4294967296]);
/// assert_eq!(BigUint::from(112500000000u64).to_u64_digits(), vec![112500000000]);
Copy link
Member

Choose a reason for hiding this comment

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

We should add a multi-digit example, like BigUint::from(1u128 << 64).
(ditto for iter_u64_digits, and the BigInt counterparts.)

bors bot added a commit that referenced this pull request Feb 24, 2021
192: Add iter_u32_digits , iter_u64_digits, to_u64_digits to both BigInt and BigUint r=cuviper a=cuviper

This is a rebase of #158, and I also addressed my last review comments there. I did rename the iterators to `U32Digits` and `U64Digits`, but I left the `iter_` prefix on the methods so they're not confused with the similar `to_` methods.

Co-authored-by: Vincent Rouillé <vincent@speedy37.fr>
Co-authored-by: Vincent Rouillé <v.rouille@clikengo.com>
Co-authored-by: Vincent Rouillé <vincent@speedy37.fr>
Co-authored-by: Josh Stone <cuviper@gmail.com>
@cuviper
Copy link
Member

cuviper commented Feb 24, 2021

Completed in #192.

@cuviper cuviper closed this Feb 24, 2021
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.

None yet

2 participants