Alignment: move from ptr to mem and rename as_nonzero to as_nonzero_usize

[GrigorenkoPV]

View all comments

• tracking issue: Tracking Issue for ptr::Alignment type #102070
• split off from Partially stabilize ptr_alignment_type as alignment_type #153261

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @scottmcm, libs
• @scottmcm, libs expanded to 8 candidates
• Random selection from Mark-Simulacrum, joboet, scottmcm

[bjoernager]

Should the identifier not be as_non_zero_usize? The current convention with NonNull, at least, is to use non_null as the snake_case form.

[clarfonthey]

That's an interesting observation, and after looking into precedent… it could be either way. Searching for "non" in the standard library docs: https://doc.rust-lang.org/nightly/std/?search=non

Excluding false positives, we have the following which are stable:

• std::net::(various)::set_nonblocking
• (various)::{copy, swap}{, _from, _to}nonoverlapping
• std::fmt::(various)::finish_non_exhaustive

And the following unstable:

• std::mem::Alignment::as_nonzero (this one)
• (various)::{as, from, into}_non_null (tracked in Tracking Issue for box_vec_non_null #130364)
• std::hint::prefetch_{read, write}_non_temporal (tracked in Tracking Issue for hint_prefetch #146941)
• std::process::ExitStatusError::code_nonzero (tracked in Tracking Issue for ExitStatusError #84908)
• std::ptr::NonNull::<[T]>::as_non_null_ptr (tracked in Tracking Issue for raw slice getters (slice_ptr_get) #74265)
• std::intrinsics::ct{t,l}z_nonzero (perma-unstable)

So, really, the answer is that there's not really been a consistent choice, except that non_exhaustive is stable and so is nonblocking and nonoverlapping. The rest are just unstable methods.

In general… we do tend to prefer non_zero over nonzero, but we do have nonblocking and nonoverlapping that probably could be deprecated and renamed, if someone cared enough.

[Mark-Simulacrum]

cc @scottmcm on the above since you've been involved pretty heavily with this type. I'll also nominate for libs-api, it sounds like this might be on the way towards stabilizing so probably worth making a call on that bikeshed.

r=me on the impl itself

[clarfonthey]

(FWIW the bikeshed could probably be decided in the actual stabilisation thread, since the main benefit is moving the module, and that's what generated the big diff, but this method name is a relatively tiny change that can be done with stabilisation.)

[scottmcm]

Since we have a PR for this with an r=, and existing libs-api thoughts that it should be in mem, I think we should just land it and we can ask libs-api what to do about the method separately. I don't have a ton of thoughts about this -- could even just go for .get() giving NonNull<usize> because people can .get().get() if they really need it 🤷

@bors r=Mark-Simulacrum rollup=iffy (looks conflict-prone in mir blessings)

[rust-bors]

📌 Commit a724880 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

[scottmcm]

Sub-issue of #102070 for the method name discussion: #154237

[JonathanBrouwer]

@bors r-
#154240 (comment)

[rust-bors]

This pull request was unapproved.

This PR was contained in a rollup (#154240), which was unapproved.

[nia-e]

Bikeshed discussed in #154237; unnominating.

@rustbot label -I-libs-api-nominated

[clarfonthey]

Bikeshed result is no change needed to current PR, so, should be good once the conflicts are resolved. Probably needs a rebase and to verify the tests pass; there might be some tests only run in the main CI that don't show up for PRs.

[rust-bors]

This pull request was unapproved.

This PR was contained in a rollup (#154347), which was unapproved.

[rustbot]

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.

[GrigorenkoPV]

I tried to run mir-opt tests locally, but couldn't get some of the windows ones to work, so maybe a try job for tests-various (or what's it called) would make sense?

[scottmcm]

@bors try jobs=test-various

[scottmcm]

but couldn't get some of the windows ones to work

I'm not sure what you mean -- there are no windows-specific mir-opt tests as far as I know.

./x test tests/mir-opt --bless should get everything you need, as they use cross-compilation with synthetic targets.

(But if the try job comes back clean then maybe you got everything already.)

[rust-bors]

☀️ Try build successful (CI)
Build commit: 5466f15 (5466f15f9b5f6a110e3336bada0ec8f402d9163a, parent: 6d8bc65fc851433b0c17e5530ca68ff7eb41815c)

[scottmcm]

With CI and that job happy, let's try again...
@bors r+

[rust-bors]

📌 Commit ba1e06a has been approved by scottmcm

It is now in the queue for this repository.

[rust-bors]

☀️ Test successful - CI
Approved by: scottmcm
Duration: 3h 12m 8s
Pushing ac40f5e to main...

[github-actions]

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 23903d0 (parent) -> ac40f5e (this PR)

Test differences

Show 358 test diffs

358 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard ac40f5e105ddc8144904679cb63c8879708a9a26 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. dist-x86_64-apple: 1h 56m -> 2h 27m (+26.9%)
2. aarch64-apple: 3h 17m -> 2h 47m (-15.1%)
3. dist-aarch64-msvc: 1h 37m -> 1h 51m (+14.5%)
4. dist-aarch64-apple: 1h 55m -> 1h 38m (-14.2%)
5. dist-s390x-linux: 1h 27m -> 1h 38m (+13.3%)
6. tidy: 2m 49s -> 2m 28s (-12.6%)
7. dist-powerpc64-linux-musl: 1h 28m -> 1h 39m (+12.4%)
8. x86_64-gnu-aux: 2h 27m -> 2h 9m (-12.4%)
9. dist-armv7-linux: 1h 38m -> 1h 51m (+12.4%)
10. x86_64-gnu-llvm-22-1: 1h 8m -> 1h 16m (+11.9%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

[rust-timer]

Finished benchmarking commit (ac40f5e): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary 6.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	6.9%	[6.9%, 6.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary -3.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.0%	[-3.0%, -3.0%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 494.635s -> 484.633s (-2.02%)
Artifact size: 395.08 MiB -> 397.11 MiB (0.52%)