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

Arguments mixed up in documentation of rem_euclid and div_euclid #128857

Closed
fstecker opened this issue Aug 9, 2024 · 2 comments · Fixed by #129480
Closed

Arguments mixed up in documentation of rem_euclid and div_euclid #128857

fstecker opened this issue Aug 9, 2024 · 2 comments · Fixed by #129480
Assignees
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@fstecker
Copy link

fstecker commented Aug 9, 2024

Location

The documentation of rem_euclid and div_euclid for any signed integer type, e.g.: https://doc.rust-lang.org/stable/std/primitive.i32.html#method.rem_euclid

Summary

In the "Panics" section it says "This function will panic if rhs is 0 or if self is -1 and rhs is Self::MIN.", but I believe this is the wrong way around: the panic happens on MIN % -1, not on (-1) % MIN, as (correctly) explained in wrapping_rem_euclid and all other related functions.

Also, is there any situation where someone would want rem_euclid over wrapping_rem_euclid? Mathematically, MIN % -1 should really just be 0, that it panics is more of an implementation artifact (the corresponding division MIN / -1 overflows). Maybe it would be a good idea to add something like "If this is a problem, use wrapping_rem_euclid instead." to the documentation of rem_euclid? Though in most applications rhs will probably be positive anyway.

@fstecker fstecker added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Aug 9, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 9, 2024
@jieyouxu jieyouxu added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 9, 2024
@lolbinarycat
Copy link

@rustbot claim

@lolbinarycat
Copy link

the error is also present on div_floor and div_ceil.

lolbinarycat pushed a commit to lolbinarycat/rust that referenced this issue Aug 23, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 28, 2024
docs: correct panic conditions for rem_euclid and similar functions

fixes rust-lang#128857

also fixes the documentation for functions behind the `int_roundings` feature (rust-lang#88581)
@bors bors closed this as completed in d7e7886 Aug 28, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Aug 28, 2024
Rollup merge of rust-lang#129480 - lolbinarycat:euclid-docs, r=joboet

docs: correct panic conditions for rem_euclid and similar functions

fixes rust-lang#128857

also fixes the documentation for functions behind the `int_roundings` feature (rust-lang#88581)
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Aug 29, 2024
docs: correct panic conditions for rem_euclid and similar functions

fixes rust-lang/rust#128857

also fixes the documentation for functions behind the `int_roundings` feature (#88581)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants