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

Implement RFC #2169 (Euclidean modulo). #49389

Merged
merged 5 commits into from
Apr 13, 2018

Conversation

fanzier
Copy link
Contributor

@fanzier fanzier commented Mar 26, 2018

Tracking issue: #49048

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @KodrAus (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 26, 2018
Copy link
Member

@varkor varkor left a comment

Choose a reason for hiding this comment

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

This is looking good! I've left a few very minor comments, but apart from that and fixing the tidy errors, I can't see any problems!

@@ -1318,6 +1460,40 @@ $EndFeature, "
}
}


doc_comment! {
concat!("Calculates the modulo of Euclidean divsion `self.mod_euc(rhs)`.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I would call this "the remainder" or "the modulus", as "modulo" is the operation, rather than the result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing to "remainder" since "modulus" is another word for "divisor".



doc_comment! {
concat!("Calculates the modulo `self mod rhs` by Euclidean division.
Copy link
Member

Choose a reason for hiding this comment

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

And here.

doc_comment! {
concat!("Calculates the quotient of Euclidean division of `self` by `rhs`.

This computes the integer n such that `self = n * rhs + self.mod_euc(rhs)`.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: backticks around n for consistency (and in f32/f64).

#[unstable(feature = "euclidean_division", issue = "49048")]
#[inline]
pub fn wrapping_div_euc(self, rhs: Self) -> Self {
self.div_euc(rhs)
Copy link
Member

Choose a reason for hiding this comment

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

This can be the standard self / rhs to avoid the extra branch (like with wrapping_mod_euc).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks.

Copy link
Member

@varkor varkor left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@fanzier
Copy link
Contributor Author

fanzier commented Mar 27, 2018

I was just wondering whether the f32/f64 code should live in libstd or in libcore. Since it doesn't depend on anything special, I think it should be in core, not std. On the other hand, there is a lot of stuff in libstd/f32.rs that I would have guessed should be in libcore/num/f32.rs. What is the policy regarding this?

@varkor
Copy link
Member

varkor commented Mar 28, 2018

As far as I'm aware, the libstd methods are limited to those that rely on platform libraries / intrinsics (e.g. most of the mathematical methods). Pure-Rust methods tend to reside in libcore (which also makes them available for no_std). I wouldn't be surprised if the situation isn't entirely consistent at the moment.

I think libcore was the correct choice here.

Edit: just realised the floating-point methods are not in libcore right now. See later comment.

@fanzier
Copy link
Contributor Author

fanzier commented Mar 28, 2018

@varkor That makes sense. So I should move the float methods to libcore?

Edit: Apparently, to move it there, I would have to add the div_euc and mod_euc methods to the num::Float trait, which seems like overkill. Let's keep it as is?

@varkor
Copy link
Member

varkor commented Mar 28, 2018

I got a little confused before, sorry! I think it would be better to move the methods to libcore and delegate to them in libstd, yes (see, for example, the relatively recent to_bits, which does the same). You will have to extend the Float trait, but this should also allow the methods to be used on no_std, which is useful and expected. (It's not like the Float trait is being kept deliberately minimal as far as I can tell.)

@fanzier
Copy link
Contributor Author

fanzier commented Mar 28, 2018

Fair enough, I'll move it into libcore. But since div_euc uses f32::trunc, should I move trunc from libstd into num::Float as well, or use intrinsics::truncf32 directly instead?

@varkor
Copy link
Member

varkor commented Mar 28, 2018

But since div_euc uses f32::trunc, should I move trunc from libstd into num::Float as well, or use intrinsics::truncf32 directly instead?

Ah, I didn't spot that. In that case, let's just leave it in libstd for now. If anyone needs it in libcore later on, it can be moved then (unless anyone else has some strong opinions now?). I think the integer methods are the most important ones, anyway.

@shepmaster
Copy link
Member

Ping from triage, @KodrAus ! Will you have time to review soon?

Copy link
Contributor

@KodrAus KodrAus 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 late review! This is looking good, thanks @fanzier. I've just left a few nitpicky comments about the docs. Once we're happy with those I think we can push this through.

concat!("Wrapping Euclidean division. Computes `self.div_euc(rhs)`,
wrapping around at the boundary of the type.

The only case where such wrapping can occur is when one divides `MIN / -1` on a signed type (where
Copy link
Contributor

Choose a reason for hiding this comment

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

The wording here feels a little wonky. How about:

Wrapping will occur in MIN / -1 on a signed type (where MIN is the negative minimal value for the type). This is equivalent to -MIN, a positive value that is too large to represent in the type. In this case, this method returns MIN itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I almost copy-pasted that documentation from wrapping_div. Should I update the documentation there as well or should that be a separate PR?

concat!("Wrapping Euclidean modulo. Computes `self.mod_euc(rhs)`, wrapping around at the
boundary of the type.

Such wrap-around never actually occurs mathematically; implementation artifacts make `x % y`
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above. How about:

Wrapping will occur in MIN % -1 on a signed type (where MIN is the negative minimal value for the type). In this case, this method return 0.

concat!("Calculates the quotient of Euclidean division `self.div_euc(rhs)`.

Returns a tuple of the divisor along with a boolean indicating whether an arithmetic overflow would
occur. If an overflow would occur then self is returned.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should put self in backticks here.

@KodrAus
Copy link
Contributor

KodrAus commented Apr 13, 2018

Thanks @fanzier this looks good to me! I'll leave a note on the tracking issue about libcore vs libstd.

@bors r+

@bors
Copy link
Contributor

bors commented Apr 13, 2018

📌 Commit ca4e458 has been approved by KodrAus

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 13, 2018
@bors
Copy link
Contributor

bors commented Apr 13, 2018

⌛ Testing commit ca4e458 with merge f9f9050...

bors added a commit that referenced this pull request Apr 13, 2018
Implement RFC #2169 (Euclidean modulo).

Tracking issue: #49048
@bors
Copy link
Contributor

bors commented Apr 13, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: KodrAus
Pushing f9f9050 to master...

@vigna
Copy link

vigna commented Feb 27, 2023

Is the compiler able to specialize to Euclidean modulus for powers of two?

One of the reasons the Euclidean definition is the right one (unfortunately, Rust committed already to the weird C definition) is that a % (1 << k) can be computed with a mask both for signed and unsigned values. With the wrong definition and signed variables, you need to do a number of checks (that's why in Java you have to write all strength-reduction optimizations manually—the compiler will never be allowed to replace a % 8 with a & 7).

It would be nice to know I can compute remainders modulo a power of two in all cases and be sure they will be reduced to a logical bit operation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants