core: Improve the documentation for funnel shifts#154150
core: Improve the documentation for funnel shifts#154150tgross35 wants to merge 3 commits intorust-lang:mainfrom
Conversation
rustbot
added
S-waiting-on-review
|
r? @joboet rustbot has assigned @joboet. Use Why was this reviewer chosen?The reviewer was selected based on:
|
tgross35
force-pushed
the
funnel-updates
branch
3 times, most recently
from
8683922 to
5ae8d71
Compare
|
Cc @clarfonthey since you requested some of this at #145686 (comment) |
|
Output since it's impossible to read the macros: 
|
tgross35
force-pushed
the
funnel-updates
branch
from
5ae8d71 to
7a02613
Compare
|
Some ascii art I sketched for this but decided to exclude: // high low
// |0x1234 0x5678| <- shift left 12 bits
// \ /
// \--| /
// |0x4567|
// result
//
// high low
// |0x1234 0x5678| -> shift right 12 bits
// \ /
// \ |--/
// |0x2345|
// result |
library/core/src/num/uint_macros.rs
Outdated
| without modifying the original"] | ||
| #[inline(always)] | ||
| pub const fn funnel_shr(self, rhs: Self, n: u32) -> Self { | ||
| pub const fn funnel_shr(self, low: Self, n: u32) -> Self { |
There was a problem hiding this comment.
Personally I think "right" gives a better intuition here than "low". This is about the number as a bitstring after all, not about it's numeric value.
There was a problem hiding this comment.
Easy enough, renamed
There was a problem hiding this comment.
This extends into the text though -- there's still talk about the "upper half" and "lower half" of the result.
There was a problem hiding this comment.
I did consider changing that portion but decided against it. To me at least, "upper half of a u128" is pretty unambiguous and unlikely to confuse anybody, whereas "right half of a u128" sounds more like how I would describe an endian-dependent memory layout.
(The args seem fine because that's cohesive with right/left shift, the ambiguity is only when talking about widening/narrowing as a different size)
There was a problem hiding this comment.
Well, but then it is odd to call the 2nd argument right if you're not consistently using that terminology.
I know upper/lower is natural to some people, though I personally have to always pause and translate it into left/right.
There was a problem hiding this comment.
Honestly would prefer to just keep the naming of LHS and RHS and just clarify what x FSH[LR] y means instead of renaming arguments.
tgross35
force-pushed
the
funnel-updates
branch
from
7a02613 to
fd783ae
Compare
`rhs` is confusing since for the `Shl` and `Shr` traits, that is the name used for the value to shift by (i.e. `self >> rhs`).
It's a bit easier to understand the results of shifts and otherwise bitwise operations when leading zeros are present.
Shorten the summary line, cover shifts by zeros, rotates, and panics in examples, and turn the "Panics" section into a complete sentence.
tgross35
force-pushed
the
funnel-updates
branch
from
fd783ae to
aa98b14
Compare
| #[doc = concat!("assert_eq!(a.funnel_shl(0, ", $rot, "), a << ", $rot, ");")] | ||
| /// | ||
| /// // Shifting by 0 returns `self` unchanged | ||
| #[doc = concat!("assert_eq!(a.funnel_shl(0, 0), a);")] |
There was a problem hiding this comment.
| #[doc = concat!("assert_eq!(a.funnel_shl(0, 0), a);")] | |
| #[doc = concat!("assert_eq!(a.funnel_shl(b, 0), a);")] |
Or really "nonzero" value perhaps? It feels a little confusing otherwise since technically the shift doesn't actually matter if the first argument is zero already...
|
☔ The latest upstream changes (presumably #154289) made this pull request unmergeable. Please resolve the merge conflicts. |
6 participants
Change the variable name to make things a bit more clear, then rework funnel shift documentation. Examples now cover shifting by zero and emulating rotates.
Tracking issue: #145686