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

Stablize Euclidean Modulo (feature euclidean_division) #61884

Merged
merged 1 commit into from Jul 26, 2019

Conversation

@crlf0710
Copy link
Contributor

commented Jun 16, 2019

Closes #49048

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

commented Jun 16, 2019

r? @shepmaster

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

@Centril Centril added the relnotes label Jun 16, 2019

@Centril Centril added this to the 1.37 milestone Jun 16, 2019

@Centril

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

@rust-highfive rust-highfive assigned sfackler and unassigned shepmaster Jun 16, 2019

@crlf0710

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2019

ping @sfackler

@Centril

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

@rust-highfive rust-highfive assigned dtolnay and unassigned sfackler Jul 2, 2019

@Centril Centril modified the milestones: 1.37, 1.38 Jul 2, 2019

@crlf0710 crlf0710 force-pushed the crlf0710:stablize_euc branch from c22baee to 72ac8ce Jul 7, 2019

@crlf0710

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

Rebased and updated the version numbers to 1.38.

@est31

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2019

ping @dtolnay

@dtolnay

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

// in libcore
impl {i8, i16, i32, i64, i128, isize, u8, u16, u32, u64, u128, usize} {
    pub fn div_euclid(self, rhs: Self) -> Self;
    pub fn rem_euclid(self, rhs: Self) -> Self;
    pub fn wrapping_div_euclid(self, rhs: Self) -> Self;
    pub fn wrapping_rem_euclid(self, rhs: Self) -> Self;
    pub fn checked_div_euclid(self, rhs: Self) -> Option<Self>;
    pub fn checked_rem_euclid(self, rhs: Self) -> Option<Self>;
    pub fn overflowing_div_euclid(self, rhs: Self) -> (Self, bool);
    pub fn overflowing_rem_euclid(self, rhs: Self) -> (Self, bool);
}

// in libstd
impl {f32, f64} {
    pub fn div_euclid(self, rhs: Self) -> Self;
    pub fn rem_euclid(self, rhs: Self) -> Self;
}

Notes:

  • Check out the documentation of div_euclid and rem_euclid for an explanation of the behavior of Euclidean division.
  • The wrapping / checked / overflowing methods have the same signatures as the non-_euclid versions.
  • The float methods being only available in libstd is discussed in #49389 (comment). It seems to do with f32::trunc being in libstd only. We can move to libcore later on but nobody has needed that so far; mostly people care about the integer methods.

@rust-lang/libs
@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

commented Jul 12, 2019

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot

This comment has been minimized.

Copy link

commented Jul 15, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot

This comment has been minimized.

Copy link

commented Jul 25, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@Centril

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

@bors r=dtolnay,Centril rollup

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

📌 Commit 72ac8ce has been approved by dtolnay,Centril

@Centril Centril removed the needs-fcp label Jul 25, 2019

Centril added a commit to Centril/rust that referenced this pull request Jul 25, 2019

Rollup merge of rust-lang#61884 - crlf0710:stablize_euc, r=dtolnay,Ce…
…ntril

Stablize Euclidean Modulo (feature euclidean_division)

Closes rust-lang#49048

bors added a commit that referenced this pull request Jul 25, 2019

Auto merge of #62990 - Centril:rollup-k9n0hvs, r=Centril
Rollup of 15 pull requests

Successful merges:

 - #60066 (Stabilize the type_name intrinsic in core::any)
 - #60938 (rustdoc: make #[doc(include)] relative to the containing file)
 - #61884 (Stablize Euclidean Modulo (feature euclidean_division))
 - #61890 (Fix some sanity checks)
 - #62528 (Add joining slices of slices with a slice separator, not just a single item)
 - #62707 (Add tests for overlapping explicitly dropped locals in generators)
 - #62735 (Turn `#[global_allocator]` into a regular attribute macro)
 - #62822 (Improve some pointer-related documentation)
 - #62887 (Make the parser TokenStream more resilient after mismatched delimiter recovery)
 - #62921 (Add method disambiguation help for trait implementation)
 - #62930 (Add test for #51559)
 - #62942 (Use match ergonomics in Condvar documentation)
 - #62977 (Fix inconsistent highlight blocks.)
 - #62978 (Remove `cfg(bootstrap)` code for array implementations)
 - #62981 (Add note suggesting to borrow a String argument to find)

Failed merges:

 - #62964 (clarify and unify some type test names)

r? @ghost

@bors bors merged commit 72ac8ce into rust-lang:master Jul 26, 2019

3 checks passed

pr Build #20190707.12 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
@getreu

This comment has been minimized.

Copy link

commented Jul 27, 2019

modulo of a negative number is expected to be positive

mathematically this implementation does not give correct results for negative numbers. Here the explanation:

Computer languages and libraries are notoriously inconsistent, or at
least unmathematical, in their implementation of "mod" for negative
numbers. There are several ways it can be interpreted in such cases,
and the choice generally made is not what a mathematician would
probably have made. The issue is what range of values the function
should return. Mathematically, we define "modulo" not as a function,
but as a relation: any two numbers a and b are congruent modulo m if
(a - b) is a multiple of m. If we want to make a function of this, we
have to choose which number b, of all those that are congruent to a,
should be returned.

Properly, the modulus operator a mod b should be mathematically
defined as the number in the range [0,b) that is congruent to a, as
stated here:

http://mathworld.wolfram.com/ModulusCongruence.html   

In many computer languages (such as FORTRAN or Mathematica), the 
common residue of b (mod m) is written mod(b,m) (FORTRAN) or 
Mod[b,m] (Mathematica).

http://mathworld.wolfram.com/CommonResidue.html   

The value of b, where a=b (mod m), taken to be nonnegative and 
smaller than m.

Unfortunately, this statement about FORTRAN, and implicitly about the
many languages that have inherited their mathematical libraries from
FORTRAN, including C++, is not quite true where negative numbers are
concerned.

The problem is that people tend to think of modulus as the same as
remainder, and they expect the remainder of, say, -5 divided by 3 to
be the same as the remainder of 5 divided by 3, namely 2, but negated,
giving -2. We naturally tend to remove the sign, do the work, and put
the sign back on, because that's how we divide. In other words, we
expect to truncate toward zero for both positive and negative numbers,
and have the remainder be what's left "on the outside," away from
zero. More particularly, computers at least since the origin of
FORTRAN have done integer division by "truncating toward zero," so
that 5/2 = 2 and -5/2 = -2, and they keep their definition of "mod" or
"%" consistent with this by requiring that

(a/b)*b + a%b = a

so that "%" is really defined as the remainder of integer division as
defined in the language.

Because FORTRAN defined division and MOD this way, computers have
tended to follow this rule internally (in order to implement FORTRAN
efficiently), and so other languages have perpetuated it as well. Some
languages have been modified more recently to include the more
mathematical model as an alternative; in fact, FORTRAN 90 added a new
MODULO function that is defined so that the sign of MODULO(a,b) is
that of b, whereas the sign of MOD(a,b) is that of a. This makes it
match the mathematical usage, at least when b is positive.

Similarly, in Ada there are two different operators, "mod" (modulus)
and "rem" (remainder). Here's an explanation with plenty of detail:

Ada '83 Language Reference Manual - U.S. Government
http://archive.adaic.com/standards/83lrm/html/lrm-04-05.html   

Integer division and remainder are defined by the relation 

    A = (A/B)*B + (A rem B) 

where (A rem B) has the sign of A and an absolute value less than 
the absolute value of B. Integer division satisfies the identity 

    (-A)/B = -(A/B) = A/(-B) 

The result of the modulus operation is such that (A mod B) has the 
sign of B and an absolute value less than the absolute value of B; 
in addition, for some integer value N, this result must satisfy 
the relation 

    A = B*N + (A mod B) 

...
For positive A and B, A/B is the quotient and A rem B is the 
remainder when A is divided by B. The following relations are 
satisfied by the rem operator: 

    A    rem (-B) =   A rem B
    (-A) rem   B  = -(A rem B) 

For any integer K, the following identity holds: 

    A  mod   B  =  (A + K*B) mod B 

The relations between integer division, remainder, and modulus are
illustrated by the following table: 

A B A/B A rem B A mod B A B A/B A rem B A mod B

10 5 2 0 0 -10 5 -2 0 0
11 5 2 1 1 -11 5 -2 -1 4
12 5 2 2 2 -12 5 -2 -2 3
13 5 2 3 3 -13 5 -2 -3 2
14 5 2 4 4 -14 5 -2 -4 1

10 -5 -2 0 0 -10 -5 2 0 0
11 -5 -2 1 -4 -11 -5 2 -1 -1
12 -5 -2 2 -3 -12 -5 2 -2 -2
13 -5 -2 3 -2 -13 -5 2 -3 -3
14 -5 -2 4 -1 -14 -5 2 -4 -4

So what's the conclusion? There are basically two models, reasonably
distinguished in Ada terms as Remainder and Mod; the C++ "%" operator
is really Remainder, not Mod, despite what it's often called.
Actually, its behavior for negative numbers is not even defined
officially; like many things in C, it's left to be processor-dependent
because C does not define how a processor should handle integer
division. Just by chance, all compilers I know truncate integers
toward zero, and therefore treat "%" as remainder, following the
precedent of FORTRAN. As C: A Reference Manual, by Harbison and
Steele, says, "For maximum portability, programs should therefore
avoid depending on the behavior of the remainder operator when
applied to negative integral operands.
"

@crlf0710

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

@getreu i think you're talking about the built-in '%' operator, where the result of (-5) % 3 and (-5) % (-3) are both -2.

According to https://en.wikipedia.org/wiki/Euclidean_division, the expected formula here, for *_euclid, is:
a = bq + r
and
0 ≤ r < |b| .

And you can just use what you need.

@crlf0710 crlf0710 deleted the crlf0710:stablize_euc branch Jul 27, 2019

@est31

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

Please see the RFC here as well as its discussion thread for why euclidean modulo ended up the way it ended up: rust-lang/rfcs#2169

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.