impl more traits for ptr::Alignment, add mask method #115249

clarfonthey · 2023-08-26T16:11:22Z

Changes:

Adds rustc_const_unstable attributes where missing
Makes log2 method const
Adds mask method
Implements Default, which is equivalent to Alignment::MIN

No longer included in PR:

Removes indirection of AlignmentEnum type alias (this was intentional)
Implements Display, Binary, Octal, LowerHex, and UpperHex (should go through libs-api instead)
Controversially implements LowerExp and UpperExp using p instead of e to indicate a power of 2 (also should go through libs-api)

Tracking issue for ptr::Alignment: #102070

rustbot · 2023-08-26T16:11:29Z

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

clarfonthey · 2023-08-26T16:14:37Z

library/core/src/ptr/alignment.rs

+}
+
+impl Alignment {
+    // longest string: 1p-128


even though no targets have 128-bit usize, I figured I might as well future-proof it

scottmcm · 2023-08-27T08:25:54Z

library/core/src/ptr/alignment.rs

+    #[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) })


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.

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.

scottmcm · 2023-08-27T08:27:55Z

library/core/src/ptr/alignment.rs

+#[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)


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.

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.

library/core/src/ptr/alignment.rs

scottmcm · 2023-10-13T15:59:46Z

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

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

bors · 2023-10-14T00:03:47Z

⌛ Testing commit 5dda0c9 with merge 2c4b73c...

impl more traits for ptr::Alignment, add mask method Changes: * Adds `rustc_const_unstable` attributes where missing * Makes `log2` method const * Adds `mask` method * Implements `Default`, which is equivalent to `Alignment::MIN` No longer included in PR: * Removes indirection of `AlignmentEnum` type alias (this was intentional) * Implements `Display`, `Binary`, `Octal`, `LowerHex`, and `UpperHex` (should go through libs-api instead) * Controversially implements `LowerExp` and `UpperExp` using `p` instead of `e` to indicate a power of 2 (also should go through libs-api) Tracking issue for `ptr::Alignment`: rust-lang#102070

bors · 2023-10-14T00:56:01Z

💔 Test failed - checks-actions

scottmcm · 2023-10-14T01:01:50Z

library/core/src/ptr/alignment.rs

+    /// assert_eq!(four.mask(Alignment::of::<u8>().mask()), four);
+    /// assert_eq!(four.mask(Alignment::of::<u16>().mask()), four);
+    /// assert_eq!(four.mask(Alignment::of::<u32>().mask()), four);
+    /// assert_ne!(four.mask(Alignment::of::<u64>().mask()), four);


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-

clarfonthey · 2023-10-15T18:34:04Z

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.

clarfonthey · 2023-10-28T05:16:42Z

Ping @scottmcm when you have time to take a look at this

clarfonthey · 2023-11-18T05:04:18Z

Ping @scottmcm again when you're free.

scottmcm · 2023-11-18T05:14:38Z

Oops, thanks for the ping!

@bors r+ rollup=iffy (had a per-target failure last time)

bors · 2023-11-18T05:14:40Z

📌 Commit 114873d has been approved by scottmcm

It is now in the queue for this repository.

bors · 2023-11-18T06:51:18Z

⌛ Testing commit 114873d with merge 61d3b26...

bors · 2023-11-18T08:47:54Z

☀️ Test successful - checks-actions
Approved by: scottmcm
Pushing 61d3b26 to master...

rust-timer · 2023-11-18T10:22:46Z

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%)

rustbot assigned cuviper Aug 26, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 26, 2023

clarfonthey commented Aug 26, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

clarfonthey force-pushed the alignment branch from bdb7191 to a6546fe Compare August 26, 2023 17:39

This comment has been minimized.

Sign in to view

clarfonthey force-pushed the alignment branch from a6546fe to e86b2ed Compare August 26, 2023 19:28

This comment has been minimized.

Sign in to view

clarfonthey force-pushed the alignment branch from e86b2ed to dedf2f2 Compare August 27, 2023 02:35

This comment has been minimized.

Sign in to view

clarfonthey force-pushed the alignment branch from dedf2f2 to a3c430d Compare August 27, 2023 03:11

This comment has been minimized.

Sign in to view

scottmcm reviewed Aug 27, 2023

View reviewed changes

library/core/src/ptr/alignment.rs Show resolved Hide resolved

clarfonthey force-pushed the alignment branch 2 times, most recently from ac74183 to f8cd316 Compare August 27, 2023 20:10

This comment has been minimized.

Sign in to view

clarfonthey force-pushed the alignment branch from f8cd316 to 9227f94 Compare September 7, 2023 02:23

clarfonthey force-pushed the alignment branch from 9227f94 to 26b97a2 Compare September 16, 2023 05:32

clarfonthey force-pushed the alignment branch from 26b97a2 to 5c7f096 Compare October 13, 2023 06:13

rustbot assigned scottmcm and unassigned cuviper Oct 13, 2023

rustbot added S-waiting-on-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 Oct 13, 2023

scottmcm mentioned this pull request Oct 13, 2023

Tracking Issue for ptr::Alignment type #102070

Open

6 tasks

clarfonthey force-pushed the alignment branch from 6651cb6 to 644e025 Compare October 13, 2023 20:50

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 Oct 13, 2023

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review 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 Oct 14, 2023

scottmcm reviewed Oct 14, 2023

View reviewed changes

bors added S-waiting-on-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 Oct 14, 2023

clarfonthey force-pushed the alignment branch from 1d840e2 to 3a8ec1c Compare November 3, 2023 00:34

Dylan-DPC added S-waiting-on-review 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 Nov 6, 2023

impl more traits for ptr::Alignment, add mask method

114873d

clarfonthey force-pushed the alignment branch from 3a8ec1c to 114873d Compare November 18, 2023 05:05

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 Nov 18, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 18, 2023

bors merged commit 61d3b26 into rust-lang:master Nov 18, 2023
12 checks passed

rustbot added this to the 1.76.0 milestone Nov 18, 2023

clarfonthey deleted the alignment branch November 18, 2023 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

impl more traits for ptr::Alignment, add mask method #115249

impl more traits for ptr::Alignment, add mask method #115249

clarfonthey commented Aug 26, 2023 •

edited

rustbot commented Aug 26, 2023

clarfonthey Aug 26, 2023

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.

scottmcm Aug 27, 2023

clarfonthey Aug 27, 2023

scottmcm Aug 27, 2023

clarfonthey Aug 27, 2023

This comment has been minimized.

scottmcm commented Oct 13, 2023

bors commented Oct 14, 2023

This comment has been minimized.

bors commented Oct 14, 2023

scottmcm Oct 14, 2023

clarfonthey commented Oct 15, 2023

clarfonthey commented Oct 28, 2023

clarfonthey commented Nov 18, 2023

scottmcm commented Nov 18, 2023

bors commented Nov 18, 2023

bors commented Nov 18, 2023

bors commented Nov 18, 2023

rust-timer commented Nov 18, 2023

impl more traits for ptr::Alignment, add mask method #115249

impl more traits for ptr::Alignment, add mask method #115249

Conversation

clarfonthey commented Aug 26, 2023 • edited

rustbot commented Aug 26, 2023

Choose a reason for hiding this comment

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.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

scottmcm commented Oct 13, 2023

bors commented Oct 14, 2023

This comment has been minimized.

bors commented Oct 14, 2023

Choose a reason for hiding this comment

clarfonthey commented Oct 15, 2023

clarfonthey commented Oct 28, 2023

clarfonthey commented Nov 18, 2023

scottmcm commented Nov 18, 2023

bors commented Nov 18, 2023

bors commented Nov 18, 2023

bors commented Nov 18, 2023

rust-timer commented Nov 18, 2023

Overall result: no relevant changes - no action needed

clarfonthey commented Aug 26, 2023 •

edited