[WIP] Remove various transmutes by nvzqz · Pull Request #47005 · rust-lang/rust

nvzqz · 2017-12-25T22:56:32Z

This PR removes various transmutes that can either be replaced with casts or safe code.

rust-highfive · 2017-12-25T22:56:37Z

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

petrochenkov · 2017-12-26T22:13:40Z

I don't know what is the official policy and half of the changed cases looks better with transmute to me.
r? @bluss

bluss · 2017-12-31T22:17:13Z

This one in particular I don't know why transmute isn't the best way. We need some way to do the &[X] -> &NewType([X]) cast.

This still performs the same cast, just without transmuting the fat pointer. Can you explain further why transmute would be preferred here?

bluss · 2017-12-31T22:18:14Z

Yes, we want to set a good example and not use transmute, though I agree many of them look better with transmute; with transmute, we also have some size checking that the pointer casts don't have.
But we don't have any pointee size check in either case.

So it's almost like we want better primitives that are transmute-like. Previous removal of them was in #14069 but there are frankly not many there that are worth bringing back, except transmute_lifetime, and whatever new ones we need.

shepmaster · 2018-01-06T02:30:53Z

Happy new year from triage, @nvzqz! A reminder that this is awaiting your feedback!

bors · 2018-01-06T05:23:19Z

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

nvzqz · 2018-01-07T00:10:55Z

@bluss I'm of the opinion that mem::transmute should be reserved for certain cases, such as ones that warrant a size check. The nomicon states that it's very easy to cause UB via transmutes, and I think that the Rust standard library/ies should be somewhat of a role model in how it uses them.

I agree that some of these look better as transmutes but unless there's necessary size checks, alternatives should be considered.

Proof I don't hate mem::transmute. 😜

@shepmaster Happy new year ❤️

Edit: This is still a WIP. I'll update the title when I feel that this PR finishes accomplishing what I intend for it to. I've just been busy in my personal life.

bluss · 2018-01-07T01:39:37Z

@nvzqz Sure, I'm also partial to that. I don't really dislike these changes, but I hope we can find something even better for Rust than these myriads of double as casts.

bluss · 2018-01-07T01:45:38Z

This one is actually wrong — the original is tricky. self.addr.sun_path is an array and the transmute with explicit type is using the coercion from array to slice.
I am not actually sure if it's wrong. But I'll have to double check how this works with coercion here.
Reinterpreting &[u8] <-> &[c_char] seems like it should be a function.

And the old target type was &[u8], in this code it is instead &[c_char]. They should switch places. Convert from c_char to u8.

I see what you mean. Thanks a lot @bluss, you're awesome 👍🏻. Thanks for everything you do for this project. I'm make the appropriate changes tomorrow.

Fixes lifetime errors that occur when using the AsRef implementation.

shepmaster · 2018-01-13T00:41:07Z

Heads up from triage, @bluss! Looks like there are new commits to be reviewed!

eddyb · 2018-01-15T00:42:10Z

 fn deref(&self) -> &'static str {
 let l = INTERNER.strs.lock().unwrap();
- unsafe { mem::transmute::<&str, &'static str>(l.get(*self)) }
+ let s: &str = l.get(*self).as_ref();


What's as_ref called on? If it's &String then you should remove .as_ref() and let it coerce.

Same for all the other 5 cases in this file.

eddyb · 2018-01-15T00:44:31Z

 unsafe {
- mem::transmute(slice::from_raw_parts(0x1 as *const T, 0))
+ let slice = slice::from_raw_parts(0x1 as *const T, 0);
+ &*(slice as *const [T] as *const Slice<T>)


This should be &*(mem::align_of::<T>() as *const [T; 0] as *const [T] as *const Slice<T>).

shepmaster · 2018-01-26T18:06:53Z

Ping from triage @nvzqz ! We haven't heard from you in over a week; do you think you'll be able to respond to the feedback soon?

shepmaster · 2018-02-03T01:42:02Z

Thanks for the PR, @nvzqz ! Since we haven't heard from you in a few weeks, I'm going to go ahead and close this PR for now. Please feel free to reopen when you are able to address the last bit of feedback.

rust-highfive assigned petrochenkov Dec 25, 2017

nvzqz force-pushed the remove-transmute branch from 729ce39 to a2f719fCompare December 25, 2017 23:19

rust-highfive assigned bluss and unassigned petrochenkov Dec 26, 2017

kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 27, 2017

bluss reviewed Dec 31, 2017
View reviewed changes

bluss reviewed Jan 7, 2018
View reviewed changes

nvzqz force-pushed the remove-transmute branch from dde1d39 to dd49ccdCompare January 7, 2018 20:13

Replace many transmutes with casts or safe code
90a17e9

nvzqz force-pushed the remove-transmute branch from dd49ccd to 90a17e9Compare January 7, 2018 22:08

nvzqz added 5 commits January 7, 2018 17:22

Add missing 'as' in casts
071fe7e

Swap Deref and AsRef impls for Interned<PathBuf>
d70bda8

Manually implement Deref for Interned<PathBuf>
5dfdaa8
Fixes lifetime errors that occur when using the AsRef implementation.

Add missing unsafe blocks to raw pointer casts
284bdea

Fix mutability of array reference
8937356

shepmaster 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 Jan 13, 2018

eddyb reviewed Jan 15, 2018
View reviewed changes

shepmaster 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 Jan 20, 2018

shepmaster closed this Feb 3, 2018

shepmaster added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 3, 2018

Uh oh!

Uh oh!

Conversation

nvzqz commented Dec 25, 2017

Uh oh!

rust-highfive commented Dec 25, 2017

Uh oh!

petrochenkov commented Dec 26, 2017

Uh oh!

blussDec 31, 2017

Choose a reason for hiding this comment

Uh oh!

nvzqzJan 1, 2018

Choose a reason for hiding this comment

Uh oh!

bluss commented Dec 31, 2017

Uh oh!

shepmaster commented Jan 6, 2018

Uh oh!

bors commented Jan 6, 2018

Uh oh!

nvzqz commented Jan 7, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bluss commented Jan 7, 2018

Uh oh!

blussJan 7, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blussJan 7, 2018

Choose a reason for hiding this comment

Uh oh!

nvzqzJan 7, 2018

Choose a reason for hiding this comment

Uh oh!

shepmaster commented Jan 13, 2018

Uh oh!

eddybJan 15, 2018

Choose a reason for hiding this comment

Uh oh!

eddybJan 15, 2018

Choose a reason for hiding this comment

Uh oh!

eddybJan 15, 2018

Choose a reason for hiding this comment

Uh oh!

shepmaster commented Jan 26, 2018

Uh oh!

shepmaster commented Feb 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

nvzqz commented Jan 7, 2018•
edited
Loading

blussJan 7, 2018•
edited
Loading