Add Saturating type (based on Wrapping type)#87921

kellerkindt · 2021-08-10T17:23:24Z

Tracking #87920

Unresolved Questions

rust-highfive · 2021-08-10T17:23:27Z

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

library/core/src/num/saturating.rs

kennytm · 2021-08-12T09:29:13Z

i'm pretty sure the implementation of BitAnd, rotate_{left,right}, swap_bytes and reverse_bits are correct. the question is whether these bit-level operations are useful for Saturating, and whether they can live independent of Shl and Shr (see discussion from #87921 (comment)).

while the methods can have a separate library feature, the trait impls (BitAnd, BitOr, BitXor, Shl, Shr) must be stabilized together with Saturating.

kellerkindt · 2021-08-12T10:29:30Z

I added naive shift (and remainder) impls. What needs to ben done next?

kennytm · 2021-08-12T11:40:01Z

saturating_eng is only implemented on signed integer types

i don't think it makes sense to impl Neg for Saturating<unsigned> 😅

kellerkindt · 2021-08-12T11:45:50Z

saturating_eng is only implemented on signed integer types

i don't think it makes sense to impl Neg for Saturating<unsigned> sweat_smile

Well... you are actually right about that 😅
fixed

SkiFire13 · 2021-08-16T09:17:28Z

impl Div for Saturating<T> falls back on inner integer division - which seems alright?

I think this is wrong in case of iN::MIN / -1. I would expect a saturating operation to evaluate to iN::MAX, instead it overflows and evaluates to iN::MIN. This also seems a reason to have a saturating_div for signed integers.

library/core/src/num/int_macros.rs

library/core/src/num/saturating.rs

library/core/src/num/int_macros.rs

kellerkindt · 2021-08-19T09:10:36Z

library/core/src/num/saturating.rs

+        /// # Examples
+        ///
+        /// Basic usage:
+        ///
+        /// ```
+        /// #![feature(saturating_int_impl, saturating_div)]
+        /// use std::num::Saturating;
+        ///
+        #[doc = concat!("assert_eq!(Saturating(2", stringify!($t), "), Saturating(5", stringify!($t), ") / Saturating(2));")]
+        #[doc = concat!("assert_eq!(Saturating(", stringify!($t), "::MAX), Saturating(", stringify!($t), "::MAX) / Saturating(1));")]
+        #[doc = concat!("assert_eq!(Saturating(", stringify!($t), "::MIN), Saturating(", stringify!($t), "::MIN) / Saturating(1));")]
+        /// ```
+        ///
+        /// ```should_panic
+        /// #![feature(saturating_int_impl, saturating_div)]
+        /// use std::num::Saturating;
+        ///
+        #[doc = concat!("let _ = Saturating(0", stringify!($t), ") / Saturating(0);")]
+        /// ```
+        #[unstable(feature = "saturating_int_impl", issue = "87920")]


Not sure whether to keep this doc(-tests)

library/core/src/num/int_macros.rs

kellerkindt · 2021-08-23T08:37:36Z

What are the next steps? :)

kennytm

rest LGTM

kennytm · 2021-08-25T15:31:09Z

library/core/src/num/saturating.rs

+                if other < 0 {
+                    Saturating(self.0.shr((-other & self::shift_max::$t as $f) as u32))
+                } else {
+                    Saturating(self.0.shl((other & self::shift_max::$t as $f) as u32))
+                }


🤔 this implementation is strange.

For other >= 0, the implementation is the same as self.0 << other. This is fine.

For other < 0, however, this becomes self.0 >> -other, which is never possible for the << operation.

I expected this either performs Ⓐ self.0 << other, or Ⓑ self.0.wrapping_shl(other), or Ⓒ self.0 << other.clamp(0, self::shift_max), or Ⓓ don't implement Shl<signed> at all.

I've gone for Ⓓ because it does not seem clear (to me) what the correct impl should look like. I'd rather have someone figure it out later, than implementing it wrong now.

library/core/src/num/saturating.rs

… is clear

kennytm · 2021-08-28T14:22:30Z

thanks!

@bors r+

bors · 2021-08-28T14:22:32Z

📌 Commit ce636f2 has been approved by kennytm

bors · 2021-08-28T23:39:05Z

⌛ Testing commit ce636f2 with merge 677b517...

bors · 2021-08-29T02:21:01Z

☀️ Test successful - checks-actions
Approved by: kennytm
Pushing 677b517 to master...

Stabilize feature `saturating_div` for rust 1.58.0 The tracking issue is rust-lang#89381 This seems like a reasonable simple change(?). The feature `saturating_div` was added as part of the ongoing effort to implement a `Saturating` integer type (see rust-lang#87921). The implementation has been discussed [here](rust-lang#87921 (comment)) and [here](rust-lang#87921 (comment)). It extends the list of saturating operations on integer types (like `saturating_add`, `saturating_sub`, `saturating_mul`, ...) by the function `fn saturating_div(self, rhs: Self) -> Self`. The stabilization of the feature `saturating_int_impl` (for the `Saturating` type) needs to have this stabilized first. Closes rust-lang#89381

rust-highfive assigned kennytm Aug 10, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 10, 2021

kellerkindt force-pushed the master branch from 7b7b0d7 to 3e3007e Compare August 10, 2021 17:26

Add Saturating type (based on Wrapping type)

709a6c9

kellerkindt force-pushed the master branch from 3e3007e to 709a6c9 Compare August 10, 2021 17:27

This comment has been minimized.

Sign in to view

kellerkindt added 2 commits August 10, 2021 19:51

Remove unused macros

8841155

Rustfmt

cdc90f9

This comment has been minimized.

Sign in to view

Fix missed tests

d4c9f76

kellerkindt mentioned this pull request Aug 10, 2021

Tracking Issue for Saturating type #87920

Closed

6 tasks

kennytm reviewed Aug 12, 2021

View reviewed changes

kellerkindt added 3 commits August 12, 2021 09:06

Make all the impls for Staturating unstable saturating_int_impl

631766c

Remove mentioning of modular arithmetic

6cf4dd9

Like in Wrapping use shift in doctest

57dacfe

kellerkindt added 3 commits August 12, 2021 12:08

Replace doc test with doc macro call

e240853

Add naive shift implementation to Saturating

3f7d2ce

Add naive remainder impl to Saturating

7861121

kennytm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 12, 2021

Implement Neg only for signed Saturating types

f136eea

kellerkindt added 2 commits August 19, 2021 09:59

Saturate negative division

8049230

Add and use saturating_div instead of impl inside Saturating

742d450

kellerkindt commented Aug 19, 2021

View reviewed changes

library/core/src/num/int_macros.rs Show resolved Hide resolved

kellerkindt commented Aug 19, 2021

View reviewed changes

library/core/src/num/saturating.rs Outdated Show resolved Hide resolved

kennytm reviewed Aug 19, 2021

View reviewed changes

library/core/src/num/int_macros.rs Show resolved Hide resolved

library/core/src/num/int_macros.rs Outdated Show resolved Hide resolved

library/core/src/num/int_macros.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

kellerkindt added 3 commits August 19, 2021 11:07

Add doctests to and fix saturating_div for signed integer types

6bb3aca

Add saturating_div to unsigned integer types

a0e61e2

Simplify Div impl for Saturating by using saturating_div

5ca6993

kellerkindt commented Aug 19, 2021

View reviewed changes

kennytm reviewed Aug 19, 2021

View reviewed changes

library/core/src/num/int_macros.rs Outdated Show resolved Hide resolved

Simplify saturating_div

2b5970f

kennytm reviewed Aug 25, 2021

View reviewed changes

kellerkindt added 3 commits August 28, 2021 13:28

Use wrapping shift for unsigned types

acf0a0c

Fix mentions of wrapping operations

977ae5a

Unimpl Shl{Assign} for signed Saturating types until the correct impl…

ce636f2

… is clear

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 Aug 28, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 29, 2021

bors merged commit 677b517 into rust-lang:master Aug 29, 2021

rustbot added this to the 1.56.0 milestone Aug 29, 2021

kellerkindt mentioned this pull request Sep 3, 2021

Stabilize feature saturating_div for rust 1.58.0 #88624

Merged

kellerkindt mentioned this pull request Sep 29, 2021

Tracking Issue for saturating_div #89381

Closed

2 tasks

Vic23M approved these changes Feb 12, 2022

View reviewed changes

ryanavella mentioned this pull request Mar 25, 2024

u64::saturating_div is useless #122821

Open

Uh oh!

Conversation

kellerkindt commented Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unresolved Questions

Uh oh!

rust-highfive commented Aug 10, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kennytm commented Aug 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kellerkindt commented Aug 12, 2021

Uh oh!

kennytm commented Aug 12, 2021

Uh oh!

kellerkindt commented Aug 12, 2021

Uh oh!

SkiFire13 commented Aug 16, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

kellerkindt Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kellerkindt commented Aug 23, 2021

Uh oh!

kennytm left a comment

Choose a reason for hiding this comment

Uh oh!

kennytm Aug 25, 2021

Choose a reason for hiding this comment

Uh oh!

kellerkindt Aug 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kennytm commented Aug 28, 2021

Uh oh!

bors commented Aug 28, 2021

Uh oh!

bors commented Aug 28, 2021

Uh oh!

bors commented Aug 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

kellerkindt commented Aug 10, 2021 •

edited

Loading

kennytm commented Aug 12, 2021 •

edited

Loading

kellerkindt Aug 28, 2021 •

edited

Loading