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

Stabilize euclidean modulo methods #52547

Closed

Conversation

anirudhb
Copy link
Contributor

@anirudhb anirudhb commented Jul 19, 2018

This PR stabilizes the mod_euc method on numbers.

In the RFC, the feature for mod_euc is euclidean_modulo. However, it has been implemented as euclidean_division. Should I also stabilize the div methods, or change mod_euc's feature name to euclidean_modulo?

Closes #49048

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 19, 2018
@anirudhb anirudhb changed the title Stabilize euclidean modulo methods (closes #49048) Stabilize euclidean modulo methods Jul 19, 2018
@kennytm
Copy link
Member

kennytm commented Jul 19, 2018

r? @rust-lang/libs

Note that the stabilization hasn't been FCP'ed yet.

@kennytm kennytm added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. relnotes Marks issues that should be documented in the release notes of the next release. labels Jul 19, 2018
@rust-highfive

This comment has been minimized.

@anirudhb
Copy link
Contributor Author

I don't understand the tidy error. Is it inconsistent between libstd and libcore?

@anirudhb
Copy link
Contributor Author

Oh I see, I forgot to stabilize the methods in libstd

@rust-highfive

This comment has been minimized.

@anirudhb
Copy link
Contributor Author

Ok, now can someone explain the tidy error?

@kennytm
Copy link
Member

kennytm commented Jul 19, 2018

@anirudhb {f32, f64}::div_euc.

@anirudhb
Copy link
Contributor Author

Yes, that was my question about whether I should change the feature name of Euclidean modulo to be euclidean_modulo (as defined by the RFC) or stabilize the div_euc methods.

@kennytm
Copy link
Member

kennytm commented Jul 19, 2018

Well the team will make a choice. But if neither action is taken tidy will not accept the code.

@anirudhb
Copy link
Contributor Author

OK, for now I'll change the feature name to euclidean_modulo as defined by the RFC. (I'm kinda biased towards that)

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@clarfonthey
Copy link
Contributor

I personally think that euc should be renamed to euclid in these cases, and mod should be renamed to rem for consistency.

@varkor
Copy link
Member

varkor commented Jul 20, 2018

I personally think that euc should be renamed to euclid in these cases, and mod should be renamed to rem for consistency.

A case for extending the name could be made, but renaming to rem would be inconsistent with Rust's current naming scheme (and mathematical convention). The remainder of a division operator is different to the modulo of a division.

@kennytm
Copy link
Member

kennytm commented Jul 20, 2018

and mod should be renamed to rem for consistency.

Or we could go full inconsistency renaming div_euc to quot_euc 😛

@clarfonthey
Copy link
Contributor

@varkor I beg to differ; mod_euc returns the remainder of Euclidean division. Euclidean division is defined specifically so that the modulus is the remainder. :p

In terms of renaming div to quot: it's quotrem and divmod, so having divrem and quotmod is super weird.

@anirudhb
Copy link
Contributor Author

@clarcharr @kennytm @varkor This PR is not for that. If you would like, I could close this PR and reopen it once discussion about names have been resolved.

@kennytm
Copy link
Member

kennytm commented Jul 20, 2018

@anirudhb Let's bring this to the tracking issue #49048 then. We could submit the stabilization PR after FCP is complete.

@anirudhb
Copy link
Contributor Author

Okay, since there seems to be further discussion about naming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants