Checked/wrapping/strict/unbounded funnel shifts - #161119
Conversation
rustbot
added
S-waiting-on-review
|
r? @JohnTitor rustbot has assigned @JohnTitor. Use Why was this reviewer chosen?The reviewer was selected based on:
|
tgross35
added
the
S-waiting-on-ACP
|
ACP at rust-lang/libs-team#855 |
This comment has been minimized.
This comment has been minimized.
pthariensflame
changed the title
3382d39 to
bdf9037
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
| self.wrapping_funnel_shr(right, n) | ||
| } | ||
|
|
||
| /// Performs a left funnel shift. |
There was a problem hiding this comment.
All of the summary lines say "Performs a left/right funnel shift." Please update these to be a bit more similar to the {checked,wrapping,strict,unbounded}_{shl,shr}.
| /// | ||
| /// This function will return `None` if `n` is greater than or equal to the number of | ||
| /// bits in `self`, i.e., when [`funnel_shl`](Self::funnel_shl) might panic or wrap. |
| /// This function will return `None` if `n` is greater than or equal to the number of | ||
| /// bits in `self`, i.e., when [`funnel_shl`](Self::funnel_shl) might panic or wrap. |
There was a problem hiding this comment.
Use "would" rather than "might" because we guarantee it would do one of those two things.
| /// Performs a left funnel shift. | ||
| /// | ||
| /// This function shifts by `mask(n)`, where `mask` removes any high-order bits of `n` | ||
| /// that would cause the shift to exceed the bitwidth of the type. As a result, this | ||
| /// function never panics, unlike [`funnel_shl`](Self::funnel_shl). | ||
| #[rustc_const_unstable(feature = "wrapping_funnel_shifts", issue = "161798")] | ||
| #[unstable(feature = "wrapping_funnel_shifts", issue = "161798")] | ||
| #[must_use = "this returns the result of the operation, without modifying the original"] | ||
| #[inline(always)] | ||
| pub const fn wrapping_funnel_shl(self, right: Self, n: u32) -> Self { |
There was a problem hiding this comment.
Include the "Beware that, unlike most other wrapping_* methods on integer" from wrapping_shl, adapted to funnel shifts. The result can be extra unintuitive because of the reason mentioned at rust-lang/libs-team#642 (comment), try to capture that in an example.
(same for wrapping_funnel_shr)
| /// # Panics | ||
| /// | ||
| /// This function will always panic if `n` is greater than or equal to the number of | ||
| /// bits in `self`, i.e., when [`funnel_shr`](Self::funnel_shr) might panic or wrap. | ||
| #[rustc_const_unstable(feature = "wrapping_funnel_shifts", issue = "161798")] | ||
| #[unstable(feature = "wrapping_funnel_shifts", issue = "161798")] | ||
| #[must_use = "this returns the result of the operation, without modifying the original"] | ||
| #[inline(always)] | ||
| pub const fn strict_funnel_shr(self, right: Self, n: u32) -> Self { |
There was a problem hiding this comment.
The strict_* methods have a subsection "overflow behavior". I don't love that, but this should be consistent
https://doc.rust-lang.org/nightly/std/primitive.u32.html#method.strict_shl
| #[must_use = "this returns the result of the operation, without modifying the original"] | ||
| #[inline(always)] | ||
| pub const fn strict_funnel_shr(self, right: Self, n: u32) -> Self { | ||
| assert!(n < Self::BITS, "attempt to funnel shift right with overflow"); |
There was a problem hiding this comment.
These should probably go in the overflow_panic module like the other strict_* functions to be consistent. Though I'm not really sure why that module exists.
| #[test] | ||
| fn test_wrapping_funnel_shl_overflow() { | ||
| let _ = <$T>::wrapping_funnel_shl(A, B, $T::BITS); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_wrapping_funnel_shr_overflow() { | ||
| let _ = <$T>::wrapping_funnel_shr(A, B, $T::BITS); | ||
| } | ||
|
|
There was a problem hiding this comment.
These aren't actually testing anything other than the fact that we don't panic?
These methods should be tested in test_funnel_shifts_runtime and test_funnel_shift
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
4 participants
This implements the wrapping funnel shifts requested at rust-lang/libs-team#642 (comment) and also some additional forms of funnel shift proposed at rust-lang/libs-team#855; this might interact with #161015, which is attempting to stabilize the original funnel shift methods.
The work in this PR was done entirely by some people inside a human brain, without the use of any LLMs…or much use of an IDE since ours kept crashing during this process and we had to resort to a plain text editor. 😅
This implements the tracking issue #161798.