impl more traits for ptr::Alignment, add mask method by clarfonthey · Pull Request #115249 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation29 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Changes:
- Adds
rustc_const_unstableattributes where missing - Makes
log2method const - Adds
maskmethod - Implements
Default, which is equivalent toAlignment::MIN
No longer included in PR:
- Removes indirection of
AlignmentEnumtype alias (this was intentional) - Implements
Display,Binary,Octal,LowerHex, andUpperHex(should go through libs-api instead) - Controversially implements
LowerExpandUpperExpusingpinstead ofeto indicate a power of 2 (also should go through libs-api)
Tracking issue for ptr::Alignment: #102070
r? @cuviper
(rustbot has picked a reviewer for you, use r? to override)
Status: Awaiting review from the assignee but also interested parties.
Relevant to the library team, which will review and decide on the PR/issue.
labels
| } |
|---|
| impl Alignment { |
| // longest string: 1p-128 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even though no targets have 128-bit usize, I figured I might as well future-proof it
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| #[inline] |
|---|
| pub const fn mask(self) -> usize { |
| // SAFETY: The alignment is always nonzero, and therefore decrementing won't overflow. |
| !(unsafe { self.as_usize().unchecked_sub(1) }) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly, unchecked_sub(1) is useless on unsigned types llvm/llvm-project#53377
I guess it's fine to leave it, though, since it's clearly correct. And trying to do it in isize would be UB.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's unfortunate. I even was considering at some point adding a decrement() method to NonZeroU* that could use unchecked_sub since it would never overflow, but I guess that that also wouldn't work.
| #[unstable(feature = "ptr_alignment_type", issue = "102070")] |
|---|
| impl fmt::Display for Alignment { |
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { |
| fmt::Display::fmt(&self.as_nonzero(), f) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pondering: I have no idea when we should be including Display on types. I guess it's not a problem, but it feels like alignment isn't something I'd usually be showing to people. But showing it in binary or hex seems logical, so maybe display is ok too.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that was my thought too. It's less that it's necessarily a common operation, but more that it's well-defined and not a whole lot of extra code to do. I'm fine removing it if it seems too weird.
This comment has been minimized.
r? @scottmcm
Would you be willing to split this change? I feel like it's got a bunch of things that are easy to take, but also some unprecedented things (as you note in the op), and I'd be happy to just merge the former to try to make progress here for you, since this has been open a while.
If you make this PR be these bits:
- Adds rustc_const_unstable attributes where missing
- Makes log2 method const
- Adds mask method
- Implements Default, which is equivalent to Alignment::MIN
- Please write a doc comment on it saying that it's the same as
MIN, though
- Please write a doc comment on it saying that it's the same as
Then I'd be happy to just r+ that, and we can leave the question of Display and Exp and such to libs-api for their opinions in another PR.
@rustbot author
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
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
bors added a commit to rust-lang-ci/rust that referenced this pull request
impl more traits for ptr::Alignment, add mask method
Changes:
- Adds
rustc_const_unstableattributes where missing - Makes
log2method const - Adds
maskmethod - Implements
Default, which is equivalent toAlignment::MIN
No longer included in PR:
- Removes indirection of
AlignmentEnumtype alias (this was intentional) - Implements
Display,Binary,Octal,LowerHex, andUpperHex(should go through libs-api instead) - Controversially implements
LowerExpandUpperExpusingpinstead ofeto indicate a power of 2 (also should go through libs-api)
Tracking issue for ptr::Alignment: rust-lang#102070
This comment has been minimized.
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
labels
| /// assert_eq!(four.mask(Alignment::of::().mask()), four); |
|---|
| /// assert_eq!(four.mask(Alignment::of::().mask()), four); |
| /// assert_eq!(four.mask(Alignment::of::().mask()), four); |
| /// assert_ne!(four.mask(Alignment::of::().mask()), four); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you need to not make assumptions about the alignment of primitives like this in these tests. There's no guarantee for alignof(u64), and on i686 it looks like it's 4. Maybe these demos should use Alignment::new or have specific repr(align(…)) types, rather than get them from primitives?
@bors r-
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
I don't like it, but I updated the example to have explicitly-aligned structs.
Right now, it's operating way more as a test than an example of behaviour, but that can be resolved during stabilisation.
Ping @scottmcm when you have time to take a look at this
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
Oops, thanks for the ping!
@bors r+ rollup=iffy (had a per-target failure last time)
📌 Commit 114873d has been approved by scottmcm
It is now in the queue for this repository.
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
Finished benchmarking commit (61d3b26): comparison URL.
Overall result: no relevant changes - no action needed
@rustbot label: -perf-regression
Instruction count
This benchmark run did not return any relevant results for this metric.
Max RSS (memory usage)
Results
This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | 3.9% | [2.4%, 4.9%] | 6 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | - | - | 0 |
| All ❌✅ (primary) | - | - | 0 |
Cycles
Results
This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | 2.4% | [1.8%, 2.9%] | 10 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | - | - | 0 |
| All ❌✅ (primary) | - | - | 0 |
Binary size
This benchmark run did not return any relevant results for this metric.
Bootstrap: 675.975s -> 678.192s (0.33%)
Artifact size: 313.82 MiB -> 313.84 MiB (0.01%)
Labels
This PR was explicitly merged by bors.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the library team, which will review and decide on the PR/issue.