clean up `transmute`s in `core` #110094

lukas-code · 2023-04-08T19:50:28Z

Use transmute_unchecked instead of transmute_copy for MaybeUninit::transpose.
Use manual transmute for Option<Ordering> → i8.

rustbot · 2023-04-08T19:50:36Z

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

rustbot · 2023-04-08T19:50:39Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

library/core/src/str/mod.rs

deltragon · 2023-04-08T21:47:30Z

library/core/src/mem/maybe_uninit.rs

-        // SAFETY: &[T] and &[MaybeUninit<T>] have the same layout
-        let uninit_src: &[MaybeUninit<T>] = unsafe { super::transmute(src) };
+        // SAFETY: `[T]` and `[MaybeUninit<T>]` have the same layout.
+        let uninit_src = unsafe { &*(src as *const [T] as *const [MaybeUninit<T>]) };


It might make sense to use ptr::from_ref and .cast::<T>() instead of as casts for these right away, see #109255.

Suggested change

let uninit_src = unsafe { &*(src as *const [T] as *const [MaybeUninit<T>]) };

let uninit_src = unsafe { &*ptr::from_ref(src).cast::<[MaybeUninit<T>]>() };

I agree with this change, but I'd rather land it separately. IMO, if we decide to change these pointer casts (which are everywhere in core) they should be changed all at once, preferably with a lint.

this is introducing new uses of as, which can be avoided by using those methods.

Looks like cast doesn't work for unsized pointee types yet (and won't work without moderate changes to the compiler), but I've replaced the reference -> pointer casts with ptr::from_ref now.

kadiwa4 · 2023-04-09T08:16:08Z

library/core/src/tuple.rs

@@ -129,13 +128,9 @@ const fn ordering_is_some(c: Option<Ordering>, x: Ordering) -> bool {
    // This isn't using `match` because that optimizes worse due to


It uses match now. Maybe remove/edit the comment if that's no longer a problem.

Yeah, this essentially uses

const fn ordering_is_some1(c: Option<Ordering>, x: Ordering) -> bool { x as i8 == match c { Some(c) => c as i8, None => 2, } }

instead of

const fn ordering_is_some2(c: Option<Ordering>, x: Ordering) -> bool { match c { Some(c) => c as i8 == x as i8, None => false, } }

, because the first one optimizes better.

Do you have a suggestion for a better comment?

// This is mapping None to 2 and then doing the comparison afterwards // because it optimizes better.

bors · 2023-04-14T18:23:18Z

☔ The latest upstream changes (presumably #110324) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2023-04-19T16:21:49Z

☔ The latest upstream changes (presumably #110393) made this pull request unmergeable. Please resolve the merge conflicts.

thomcc · 2023-05-05T11:19:14Z

Hm, I'm not entirely sure I see why &*(ptr::from_ref(thing) as *const T) and such is an improvement.

As a concrete example, using transmute catches compile errors in the case where destination is thin pointer and the source is a fat pointer, which is not caught by that pattern. Obviously this code doesn't have that issue, but I don't really see the justification to make the change everywhere.

Am I missing something?

lukas-code · 2023-05-05T19:32:29Z

Yeah, I'm no longer convinced, that reborrows + pointer casts are better than transmuting references either. I just noticed, that we recommend this pattern in the transmute docs and use it in most parts if the standard library already.

I can drop the reference transmute change and just keep the transmute_copy -> transmute_unchecked in MaybeUninit::transpose and manual transmute for Option<Ordering> if these look fine too you.

thomcc · 2023-05-06T16:15:34Z

Yeah, those look fine, I'd r+ it with just that.

@rustbot author

thomcc · 2023-05-06T16:16:26Z

Oh, you already did it.

@bors r+

bors · 2023-05-06T16:16:28Z

📌 Commit 7e3b934 has been approved by thomcc

It is now in the queue for this repository.

Rollup of 7 pull requests Successful merges: - rust-lang#105583 (Operand::extract_field: only cast llval if it's a pointer and replace bitcast w/ pointercast.) - rust-lang#110094 (clean up `transmute`s in `core`) - rust-lang#111150 (added TraitAlias to check_item() for missing_docs) - rust-lang#111293 (rustc --explain E0726 - grammar fixing (it's => its + add a `the` where it felt right to do so)) - rust-lang#111300 (Emit while_true lint spanning the entire loop condition) - rust-lang#111301 (Remove calls to `mem::forget` and `mem::replace` in `Option::get_or_insert_with`.) - rust-lang#111303 (update Rust Unstable Book docs for `--extern force`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned thomcc Apr 8, 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 Apr 8, 2023

asquared31415 reviewed Apr 8, 2023

View reviewed changes

library/core/src/str/mod.rs Outdated Show resolved Hide resolved

lukas-code force-pushed the less-transmute branch from 10cf175 to ebf766a Compare April 8, 2023 20:34

deltragon reviewed Apr 8, 2023

View reviewed changes

kadiwa4 reviewed Apr 10, 2023

View reviewed changes

lukas-code force-pushed the less-transmute branch from 4e83ff2 to 464c35d Compare April 14, 2023 18:56

lukas-code force-pushed the less-transmute branch from 464c35d to 9ec4999 Compare April 19, 2023 16:49

lukas-code force-pushed the less-transmute branch from 9ec4999 to f38b3d8 Compare April 28, 2023 10:15

clean up transmutes in core

7e3b934

lukas-code force-pushed the less-transmute branch from f38b3d8 to 7e3b934 Compare May 6, 2023 11:37

lukas-code changed the title ~~less transmute in core~~ clean up transmutes in core May 6, 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 May 6, 2023

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 6, 2023

JohnTitor mentioned this pull request May 7, 2023

Rollup of 7 pull requests #111311

Merged

bors merged commit aef008a into rust-lang:master May 7, 2023

rustbot added this to the 1.71.0 milestone May 7, 2023

lukas-code deleted the less-transmute branch May 7, 2023 10:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clean up `transmute`s in `core` #110094

clean up `transmute`s in `core` #110094

lukas-code commented Apr 8, 2023 •

edited

Loading

rustbot commented Apr 8, 2023

rustbot commented Apr 8, 2023

deltragon Apr 8, 2023

deltragon Apr 8, 2023

lukas-code Apr 8, 2023

the8472 Apr 10, 2023

lukas-code Apr 10, 2023

kadiwa4 Apr 9, 2023

lukas-code Apr 10, 2023

kadiwa4 Apr 19, 2023

bors commented Apr 14, 2023

bors commented Apr 19, 2023

thomcc commented May 5, 2023

lukas-code commented May 5, 2023

thomcc commented May 6, 2023

thomcc commented May 6, 2023

bors commented May 6, 2023

	let uninit_src = unsafe { &(src as const [T] as *const [MaybeUninit<T>]) };
	let uninit_src = unsafe { &*ptr::from_ref(src).cast::<[MaybeUninit<T>]>() };

		@@ -129,13 +128,9 @@ const fn ordering_is_some(c: Option<Ordering>, x: Ordering) -> bool {
		// This isn't using `match` because that optimizes worse due to

clean up transmutes in core #110094

clean up transmutes in core #110094

Conversation

lukas-code commented Apr 8, 2023 • edited Loading

rustbot commented Apr 8, 2023

rustbot commented Apr 8, 2023

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

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

bors commented Apr 14, 2023

bors commented Apr 19, 2023

thomcc commented May 5, 2023

lukas-code commented May 5, 2023

thomcc commented May 6, 2023

thomcc commented May 6, 2023

bors commented May 6, 2023

clean up `transmute`s in `core` #110094

clean up `transmute`s in `core` #110094

lukas-code commented Apr 8, 2023 •

edited

Loading