Implement `saturating_shl` and `saturating_shr` on ints by kellerkindt · Pull Request #103441 · rust-lang/rust

kellerkindt · 2022-10-23T15:31:03Z

Tracking issue #103440

Implementation based on proposed design by @avitex: https://github.com/avitex/rfcs/blob/master/text/0000-saturating-bit-shifts.md

rustbot · 2022-10-23T15:31:06Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

rust-highfive · 2022-10-23T15:31:07Z

r? @joshtriplett

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

nikic · 2022-10-23T15:41:29Z

While this is useful functionality, the chosen name is very confusing. Generally, a "saturating shift left" is understood in the same sense as a "saturating multiply", which means that (for the unsigned case) the result is MAX if any bits are shifted out. What this actually implements is a wrapping shift left with well defined semantics for shifts exceeding the bit width.

kellerkindt · 2022-10-23T15:44:39Z

Thanks for replying!

So basically, if the most significant bit is set, the return value is set to MAX otherwise a it performs a normal bit shift left operation?

CryZe · 2022-10-23T18:47:51Z

library/core/src/num/int_macros.rs

+        #[inline(always)]
+        pub const fn saturating_shr(self, rhs: u32) -> Self {
+            if rhs >= <$SelfT>::BITS {
+                0


Is 0 the correct value here? Shouldn't it do a sign extend?

Thanks, I am already working on an alternative solution with that in mind, but ./x.py test takes its time this time.

I believe this can just be self >> rhs.min(<$SelfT>::BITS) as the whole implementation, for both signed and unsigned even.

CryZe · 2022-10-23T18:54:01Z

Considering this initial implementation accidentally saturated the shift value as opposed to the resulting value (and people are similarly confused in the linked issue), it feels like either the function should be renamed to be clearer in its intention, or at least this should be very clearly pointed out in the documentation.

kellerkindt · 2022-10-23T22:30:23Z

I don't understand why the doctest fails, but outside its fine.

rust-log-analyzer · 2022-10-24T00:54:09Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

configure: rust.debug-assertions := True
configure: rust.overflow-checks := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
Attempting with retry: make prepare
---

stderr:
thread 'main' panicked at 'attempt to shift left with overflow', /checkout/library/core/src/num/mod.rs:261:5
stack backtrace:
   0:     0x56047322b9b2 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h8978151eef092d7d
   1:     0x5604732481b8 - core::fmt::write::h6d6e96066401bc0f
   2:     0x560473229cd1 - std::io::Write::write_fmt::h2ed111527b564c68
   3:     0x56047322b775 - std::sys_common::backtrace::print::he092bdc28d453324
   4:     0x56047322d1b7 - std::panicking::default_hook::{{closure}}::h9a78745f44f61a8c
   5:     0x56047322cf15 - std::panicking::default_hook::h7e5963dff20d1f30
   6:     0x56047322d9b2 - std::panicking::rust_panic_with_hook::hed0ba72ef8e79c54
   7:     0x56047322d6c1 - std::panicking::begin_panic_handler::{{closure}}::haa09fca95c2acddd
   8:     0x56047322bf5c - std::sys_common::backtrace::__rust_end_short_backtrace::h3a2229ee29684132
   9:     0x56047322d3d2 - rust_begin_unwind
  10:     0x56047320aaf3 - core::panicking::panic_fmt::h3a5b5d72039ab650
  11:     0x56047320abcd - core::panicking::panic::h12e64a76e23b19bf
  12:     0x56047320b5a0 - rust_out::main::_doctest_main_src_num_mod_rs_272_5::h32f302eb29edfec0
  13:     0x56047320b296 - rust_out::main::h4e32373a768026d2
  14:     0x56047320b1a3 - core::ops::function::FnOnce::call_once::h1c0c2d9b110d98b1
  15:     0x56047320af29 - std::sys_common::backtrace::__rust_begin_short_backtrace::h16510240dccdd97b
  16:     0x56047320af79 - std::rt::lang_start::{{closure}}::hcf05db6635b504a2
  17:     0x560473227b62 - std::rt::lang_start_internal::h83cc3d5a7bc1c2ee
  18:     0x56047320af57 - std::rt::lang_start::hdc464374a674c57a
  19:     0x56047320c178 - main
  20:     0x7ff1799efc87 - __libc_start_main
  21:     0x56047320ae3a - _start
  22:                0x0 - <unknown>

---- src/num/mod.rs - num::i16::saturating_shl (line 254) stdout ----
Test executable failed (exit status: 101).


stderr:
thread 'main' panicked at 'attempt to shift left with overflow', /checkout/library/core/src/num/mod.rs:243:5
stack backtrace:
   0:     0x561bf382b4d2 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h8978151eef092d7d
   1:     0x561bf38472a8 - core::fmt::write::h6d6e96066401bc0f
   2:     0x561bf38297f1 - std::io::Write::write_fmt::h2ed111527b564c68
   3:     0x561bf382b295 - std::sys_common::backtrace::print::he092bdc28d453324
   4:     0x561bf382ccd7 - std::panicking::default_hook::{{closure}}::h9a78745f44f61a8c
   5:     0x561bf382ca35 - std::panicking::default_hook::h7e5963dff20d1f30
   6:     0x561bf382d4d2 - std::panicking::rust_panic_with_hook::hed0ba72ef8e79c54
   7:     0x561bf382d1e1 - std::panicking::begin_panic_handler::{{closure}}::haa09fca95c2acddd
   8:     0x561bf382ba7c - std::sys_common::backtrace::__rust_end_short_backtrace::h3a2229ee29684132
   9:     0x561bf382cef2 - rust_begin_unwind
  10:     0x561bf380aa33 - core::panicking::panic_fmt::h3a5b5d72039ab650
  11:     0x561bf380ab0d - core::panicking::panic::h12e64a76e23b19bf
  12:     0x561bf380b3f5 - rust_out::main::_doctest_main_src_num_mod_rs_254_0::h1945c3c1c70b6455
  13:     0x561bf380b1d6 - rust_out::main::h4e32373a768026d2
  14:     0x561bf380b0e3 - core::ops::function::FnOnce::call_once::h1c0c2d9b110d98b1
  15:     0x561bf380ae69 - std::sys_common::backtrace::__rust_begin_short_backtrace::h16510240dccdd97b
  16:     0x561bf380aeb9 - std::rt::lang_start::{{closure}}::hcf05db6635b504a2
  17:     0x561bf3827682 - std::rt::lang_start_internal::h83cc3d5a7bc1c2ee
  18:     0x561bf380ae97 - std::rt::lang_start::hdc464374a674c57a
  19:     0x561bf380bc98 - main
  20:     0x7f25bec6fc87 - __libc_start_main
  21:     0x561bf380ad7a - _start
  22:                0x0 - <unknown>

---- src/num/mod.rs - num::i32::saturating_shl (line 259) stdout ----
Test executable failed (exit status: 101).


stderr:
thread 'main' panicked at 'attempt to shift left with overflow', /checkout/library/core/src/num/mod.rs:248:5
stack backtrace:
   0:     0x5641dfa2b492 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h8978151eef092d7d
   1:     0x5641dfa47268 - core::fmt::write::h6d6e96066401bc0f
   2:     0x5641dfa297b1 - std::io::Write::write_fmt::h2ed111527b564c68
   3:     0x5641dfa2b255 - std::sys_common::backtrace::print::he092bdc28d453324
   4:     0x5641dfa2cc97 - std::panicking::default_hook::{{closure}}::h9a78745f44f61a8c
   5:     0x5641dfa2c9f5 - std::panicking::default_hook::h7e5963dff20d1f30
   6:     0x5641dfa2d492 - std::panicking::rust_panic_with_hook::hed0ba72ef8e79c54
   7:     0x5641dfa2d1a1 - std::panicking::begin_panic_handler::{{closure}}::haa09fca95c2acddd
   8:     0x5641dfa2ba3c - std::sys_common::backtrace::__rust_end_short_backtrace::h3a2229ee29684132
   9:     0x5641dfa2ceb2 - rust_begin_unwind
  10:     0x5641dfa0a9f3 - core::panicking::panic_fmt::h3a5b5d72039ab650
  11:     0x5641dfa0aacd - core::panicking::panic::h12e64a76e23b19bf
  12:     0x5641dfa0b3ba - rust_out::main::_doctest_main_src_num_mod_rs_259_0::hf48d0d6b103afa6b
  13:     0x5641dfa0b196 - rust_out::main::h4e32373a768026d2
  14:     0x5641dfa0b0a3 - core::ops::function::FnOnce::call_once::h1c0c2d9b110d98b1
  15:     0x5641dfa0ae29 - std::sys_common::backtrace::__rust_begin_short_backtrace::h16510240dccdd97b
  16:     0x5641dfa0ae79 - std::rt::lang_start::{{closure}}::hcf05db6635b504a2
  17:     0x5641dfa27642 - std::rt::lang_start_internal::h83cc3d5a7bc1c2ee
  18:     0x5641dfa0ae57 - std::rt::lang_start::hdc464374a674c57a
  19:     0x5641dfa0bc58 - main
  20:     0x7f873e9cec87 - __libc_start_main
  21:     0x5641dfa0ad3a - _start
  22:                0x0 - <unknown>

---- src/num/mod.rs - num::i64::saturating_shl (line 265) stdout ----
Test executable failed (exit status: 101).


stderr:
thread 'main' panicked at 'attempt to shift left with overflow', /checkout/library/core/src/num/mod.rs:254:5
stack backtrace:
   0:     0x557090e2b5c2 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h8978151eef092d7d
   1:     0x557090e47398 - core::fmt::write::h6d6e96066401bc0f
   2:     0x557090e298e1 - std::io::Write::write_fmt::h2ed111527b564c68
   3:     0x557090e2b385 - std::sys_common::backtrace::print::he092bdc28d453324
   4:     0x557090e2cdc7 - std::panicking::default_hook::{{closure}}::h9a78745f44f61a8c
   5:     0x557090e2cb25 - std::panicking::default_hook::h7e5963dff20d1f30
   6:     0x557090e2d5c2 - std::panicking::rust_panic_with_hook::hed0ba72ef8e79c54
   7:     0x557090e2d2d1 - std::panicking::begin_panic_handler::{{closure}}::haa09fca95c2acddd
   8:     0x557090e2bb6c - std::sys_common::backtrace::__rust_end_short_backtrace::h3a2229ee29684132
   9:     0x557090e2cfe2 - rust_begin_unwind
  10:     0x557090e0aa33 - core::panicking::panic_fmt::h3a5b5d72039ab650
  11:     0x557090e0ab0d - core::panicking::panic::h12e64a76e23b19bf
  12:     0x557090e0b423 - rust_out::main::_doctest_main_src_num_mod_rs_265_0::hdab4f8097f9d0653
  13:     0x557090e0b1d6 - rust_out::main::h4e32373a768026d2
  14:     0x557090e0b0e3 - core::ops::function::FnOnce::call_once::h1c0c2d9b110d98b1
  15:     0x557090e0ae69 - std::sys_common::backtrace::__rust_begin_short_backtrace::h16510240dccdd97b
  16:     0x557090e0aeb9 - std::rt::lang_start::{{closure}}::hcf05db6635b504a2
  17:     0x557090e27772 - std::rt::lang_start_internal::h83cc3d5a7bc1c2ee
  18:     0x557090e0ae97 - std::rt::lang_start::hdc464374a674c57a
  20:     0x7fd554c46c87 - __libc_start_main
  21:     0x557090e0ad7a - _start
  22:                0x0 - <unknown>



---- src/num/mod.rs - num::i8::saturating_shl (line 249) stdout ----
Test executable failed (exit status: 101).

stderr:
thread 'main' panicked at 'attempt to shift left with overflow', /checkout/library/core/src/num/mod.rs:238:5
stack backtrace:
   0:     0x560fe642b452 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h8978151eef092d7d
   1:     0x560fe6447228 - core::fmt::write::h6d6e96066401bc0f
   2:     0x560fe6429771 - std::io::Write::write_fmt::h2ed111527b564c68
   3:     0x560fe642b215 - std::sys_common::backtrace::print::he092bdc28d453324
   4:     0x560fe642cc57 - std::panicking::default_hook::{{closure}}::h9a78745f44f61a8c
   5:     0x560fe642c9b5 - std::panicking::default_hook::h7e5963dff20d1f30
   6:     0x560fe642d452 - std::panicking::rust_panic_with_hook::hed0ba72ef8e79c54
   7:     0x560fe642d161 - std::panicking::begin_panic_handler::{{closure}}::haa09fca95c2acddd
   8:     0x560fe642b9fc - std::sys_common::backtrace::__rust_end_short_backtrace::h3a2229ee29684132
   9:     0x560fe642ce72 - rust_begin_unwind
  10:     0x560fe640aa33 - core::panicking::panic_fmt::h3a5b5d72039ab650
  11:     0x560fe640ab0d - core::panicking::panic::h12e64a76e23b19bf
error: doctest failed, to rerun pass `-p core --doc`
  12:     0x560fe640b3d9 - rust_out::main::_doctest_main_src_num_mod_rs_249_0::h7ac12ec6d5a80c1c
  13:     0x560fe640b1d6 - rust_out::main::h4e32373a768026d2
  14:     0x560fe640b0e3 - core::ops::function::FnOnce::call_once::h1c0c2d9b110d98b1
  15:     0x560fe640ae69 - std::sys_common::backtrace::__rust_begin_short_backtrace::h16510240dccdd97b
  16:     0x560fe640aeb9 - std::rt::lang_start::{{closure}}::hcf05db6635b504a2
  17:     0x560fe6427602 - std::rt::lang_start_internal::h83cc3d5a7bc1c2ee
  18:     0x560fe640ae97 - std::rt::lang_start::hdc464374a674c57a
  19:     0x560fe640bc18 - main
  20:     0x7f5cb7c3ec87 - __libc_start_main
  21:     0x560fe640ad7a - _start
  22:                0x0 - <unknown>

---- src/num/mod.rs - num::isize::saturating_shl (line 301) stdout ----
Test executable failed (exit status: 101).


stderr:
thread 'main' panicked at 'attempt to shift left with overflow', /checkout/library/core/src/num/mod.rs:290:5
stack backtrace:
   0:     0x562c74e2b5c2 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h8978151eef092d7d
   1:     0x562c74e47398 - core::fmt::write::h6d6e96066401bc0f
   2:     0x562c74e298e1 - std::io::Write::write_fmt::h2ed111527b564c68
   3:     0x562c74e2b385 - std::sys_common::backtrace::print::he092bdc28d453324
   4:     0x562c74e2cdc7 - std::panicking::default_hook::{{closure}}::h9a78745f44f61a8c
   5:     0x562c74e2cb25 - std::panicking::default_hook::h7e5963dff20d1f30
   6:     0x562c74e2d5c2 - std::panicking::rust_panic_with_hook::hed0ba72ef8e79c54
   7:     0x562c74e2d2d1 - std::panicking::begin_panic_handler::{{closure}}::haa09fca95c2acddd
   8:     0x562c74e2bb6c - std::sys_common::backtrace::__rust_end_short_backtrace::h3a2229ee29684132
   9:     0x562c74e2cfe2 - rust_begin_unwind
  10:     0x562c74e0aa33 - core::panicking::panic_fmt::h3a5b5d72039ab650
  11:     0x562c74e0ab0d - core::panicking::panic::h12e64a76e23b19bf
  12:     0x562c74e0b423 - rust_out::main::_doctest_main_src_num_mod_rs_301_0::h5cd7f0f6a17a9815
  13:     0x562c74e0b1d6 - rust_out::main::h4e32373a768026d2
  14:     0x562c74e0b0e3 - core::ops::function::FnOnce::call_once::h1c0c2d9b110d98b1
  15:     0x562c74e0ae69 - std::sys_common::backtrace::__rust_begin_short_backtrace::h16510240dccdd97b
  16:     0x562c74e0aeb9 - std::rt::lang_start::{{closure}}::hcf05db6635b504a2
  17:     0x562c74e27772 - std::rt::lang_start_internal::h83cc3d5a7bc1c2ee
  18:     0x562c74e0ae97 - std::rt::lang_start::hdc464374a674c57a
  19:     0x562c74e0bd88 - main
  20:     0x7f6a6c2c9c87 - __libc_start_main
  21:     0x562c74e0ad7a - _start
  22:                0x0 - <unknown>


failures:
    src/num/mod.rs - num::i128::saturating_shl (line 272)

pitaj · 2023-01-28T04:49:37Z

I believe this requires an ACP, so please create one (if you haven't already).
Then, link it here and mark this as awaiting ACP using: @rustbot label -S-waiting-on-author +S-waiting-on-ACP

@rustbot label +T-libs-api -T-libs +S-waiting-on-author -S-waiting-on-review

Dylan-DPC · 2023-02-24T18:03:46Z

@kellerkindt any updates on this?

kellerkindt · 2023-02-24T18:08:32Z

I might look into it this weekend if I find the time :)

JohnCSimon · 2023-05-28T05:22:06Z

@kellerkindt

Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you are going to continue please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

Implement saturating_shl and saturating_shr on ints

a712539

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 23, 2022

rust-highfive assigned joshtriplett Oct 23, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 23, 2022

kellerkindt changed the title ~~Implement saturating_shl and saturating_shr on ints~~ Implement saturating_shl and saturating_shr on ints Oct 23, 2022

kellerkindt mentioned this pull request Oct 23, 2022

Tracking Issue for saturating_bit_shifts #103440

Open

3 tasks

kellerkindt mentioned this pull request Oct 23, 2022

Tracking Issue for Saturating type #87920

Closed

6 tasks

CryZe reviewed Oct 23, 2022

View reviewed changes

Do sign extension for signed int shift left saturation

e6b0590

kellerkindt added 2 commits October 24, 2022 01:07

Fix saturating_shl for zero on signed integers

78c4b4f

Add more doctests for saturating_shl

3ad4b80

kellerkindt force-pushed the saturating_int_shift branch from 7db37a0 to 3ad4b80 Compare October 23, 2022 23:17

JohnCSimon closed this May 28, 2023

rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label May 28, 2023

kellerkindt mentioned this pull request May 28, 2023

Add saturating_shl and saturating_shr for ints rust-lang/libs-team#230

Closed

Uh oh!

Conversation

kellerkindt commented Oct 23, 2022

Uh oh!

rustbot commented Oct 23, 2022

Uh oh!

rust-highfive commented Oct 23, 2022

Uh oh!

nikic commented Oct 23, 2022

Uh oh!

kellerkindt commented Oct 23, 2022

Uh oh!

CryZe Oct 23, 2022

Choose a reason for hiding this comment

Uh oh!

kellerkindt Oct 23, 2022

Choose a reason for hiding this comment

Uh oh!

CryZe Oct 23, 2022

Choose a reason for hiding this comment

Uh oh!

CryZe commented Oct 23, 2022

Uh oh!

kellerkindt commented Oct 23, 2022

Uh oh!

rust-log-analyzer commented Oct 24, 2022

Uh oh!

pitaj commented Jan 28, 2023

Uh oh!

Dylan-DPC commented Feb 24, 2023

Uh oh!

kellerkindt commented Feb 24, 2023

Uh oh!

JohnCSimon commented May 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants