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 support for euclidean division and modulation #195

Merged
merged 13 commits into from Apr 30, 2022

Conversation

SparrowLii
Copy link
Contributor

Fixes #159
Add support for euclidean division and modulation.

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.

I think we can use FloatCore to avoid the cfg limitations. Otherwise, there are some style issues, but I think this looks pretty good!

src/ops/euclid.rs Outdated Show resolved Hide resolved
src/ops/euclid.rs Outdated Show resolved Hide resolved
src/ops/euclid.rs Outdated Show resolved Hide resolved
src/ops/euclid.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/ops/euclid.rs Outdated Show resolved Hide resolved
src/ops/euclid.rs Outdated Show resolved Hide resolved
src/ops/euclid.rs Outdated Show resolved Hide resolved
src/ops/euclid.rs Outdated Show resolved Hide resolved
@SparrowLii
Copy link
Contributor Author

Thanks very much for reviewing. I have made the corresponding changes.
As a Rust learner (even an open source learner), I look forward to contributing to Rust.

($($t:ty)*) => {$(
impl Euclid for $t {
#[inline]
fn div_euclid(self, v: $t) -> Self {
Copy link

Choose a reason for hiding this comment

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

Any reason for this to not use the corresponding method on all integer types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I did before. But this will cause the test of version 1.8.0 to fail, as it shows that the parameter type of div_eculid/rem_eculid is incorrect (expecting &self, not self, or on the contrary). So I implemented a separate implementation to avoid these compatibility issues.

@XAMPPRocky
Copy link
Contributor

@cuviper Is there anything left in this PR that needs to be changed? We'd really like this functionality for rust-gpu so that we can support rem_euclid on SPIR-V platforms. 🙂

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.

I think the methods should take references, for the sake of num-bigint where it's a waste of allocations to consume values. Either that, or go fully generic with a type parameter RHS and and associated type Output, so they can be implemented for any mix of values and references. There are annoyance trade-offs both ways.

src/lib.rs Outdated Show resolved Hide resolved
@SparrowLii
Copy link
Contributor Author

I think it is reasonable to use reference types from the aspect of efficiency. If a general type is used and the Output type is specified, we need to implement it separately for the several cases(self, &self, rhs, &rhs). This should probably be what a direct user-facing library should do. As a basic library, num-trait should maintain its concise style IMO

@SparrowLii SparrowLii requested a review from cuviper March 2, 2021 02:44
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.

Thanks, it looks nice.

Maybe we should also add a combined div_rem_euclid? (and checked.) That would let you get both values with the divisions and conditions only checked once. All that probably gets optimized together with primitive values anyway, but it would matter for types like BigInt.

src/ops/euclid.rs Outdated Show resolved Hide resolved
src/ops/euclid.rs Outdated Show resolved Hide resolved
@cuviper
Copy link
Member

cuviper commented Apr 30, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 30, 2022

@bors bors bot merged commit 4cd097c into rust-num:master Apr 30, 2022
bors bot added a commit to rust-num/num-bigint that referenced this pull request Aug 22, 2023
245: Implement Euclid division and remainder r=cuviper a=jaybosamiya

Now that a common trait for this has been sorted out (rust-num/num-traits#159 implemented and merged in rust-num/num-traits#195), we can now close #146 by implementing the trait for `BigInt`s. This commit does just that.

Co-authored-by: Jay Bosamiya <jaybosamiya@gmail.com>
Co-authored-by: Josh Stone <cuviper@gmail.com>
bors bot added a commit to rust-num/num-bigint that referenced this pull request Aug 22, 2023
245: Implement Euclid division and remainder r=cuviper a=jaybosamiya

Now that a common trait for this has been sorted out (rust-num/num-traits#159 implemented and merged in rust-num/num-traits#195), we can now close #146 by implementing the trait for `BigInt`s. This commit does just that.

Co-authored-by: Jay Bosamiya <jaybosamiya@gmail.com>
Co-authored-by: Josh Stone <cuviper@gmail.com>
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.

Add support for euclidean division and modulation
4 participants