Skip to content

Replace many uses of mem::transmute with more specific functions #27252

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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 10, 2015

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Jul 24, 2015

The replacements are functions that usually use a single mem::transmute in their body and restrict input and output via more concrete types than T and U. Worth noting are the transmute functions for slices and the from_utf8* family for mutable slices. Additionally, mem::transmute was often used for casting raw pointers, when you can already cast raw pointers just fine with as.

This builds upon #27233.

@rust-highfive
Copy link
Contributor

r? @pcwalton

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

@alexcrichton
Copy link
Member

I think this might be getting a little too ambitious with the additions to the public APIs of these types. The char::from_u32_unchecked function was relatively harmless but some of these here are a bit more questionable. Can this avoid expanding the public APIs of these modules?

@tbu-
Copy link
Contributor Author

tbu- commented Jul 24, 2015

@alexcrichton
Now it only adds as_bytes_mut to &mut str which has precedent with String::as_mut_vec and was probably just not there yet because we didn't deal with &mut str until recently, and it adds slice::transmute which 1) is used multiple times in the standard library and 2) is an improvement in the unsafety as it guarantees that you don't end up with a shorter or longer slice (with a bigger type you suddenly have a slice pointing into out-of-bounds area of the old slice) and that the alignment condition still holds.

@alexcrichton
Copy link
Member

Could this be split into separate PRs? Landing removal of mem::transmute without adding new functions is certainly fine to fly through, but I'm a little uncomfortable with slice::transmute and as_bytes_mut. We have very little precedent for "unsafe plus some runtime assertion" functions or functions dealing with &mut str, so there will likely be some more discussion about those aspects.

@tbu-
Copy link
Contributor Author

tbu- commented Jul 24, 2015

@alexcrichton
Now it doesn't expose any new API.

@@ -389,6 +389,7 @@ impl<K, V> Node<K, V> {

#[inline]
pub fn as_slices_internal_mut<'b>(&'b mut self) -> MutNodeSlice<'b, K, V> {
// TODO: Bad: This relies on structure layout!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this won't pass make tidy (due to the TODO). Could you remove this and perhaps it can be just handled later?

@bors
Copy link
Collaborator

bors commented Jul 26, 2015

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

@Gankra
Copy link
Contributor

Gankra commented Jul 27, 2015

Needs a rebase and travis is mad.

@tbu- tbu- force-pushed the pr_less_transmutes branch from d2ec989 to a6bd9fc Compare August 9, 2015 19:52
tbu- added 5 commits August 9, 2015 22:05
The replacements are functions that usually use a single `mem::transmute` in
their body and restrict input and output via more concrete types than `T` and
`U`. Worth noting are the `transmute` functions for slices and the `from_utf8*`
family for mutable slices. Additionally, `mem::transmute` was often used for
casting raw pointers, when you can already cast raw pointers just fine with
`as`.
@tbu- tbu- force-pushed the pr_less_transmutes branch from c8dbd85 to 2ec2668 Compare August 9, 2015 20:06
@tbu-
Copy link
Contributor Author

tbu- commented Aug 9, 2015

@alexcrichton
Sorry for the delay. I rebased it onto current master and addressed your comments.

@alexcrichton
Copy link
Member

@bors: r+ 33af24c

@bors
Copy link
Collaborator

bors commented Aug 10, 2015

⌛ Testing commit 33af24c with merge 3d69bec...

bors added a commit that referenced this pull request Aug 10, 2015
The replacements are functions that usually use a single `mem::transmute` in their body and restrict input and output via more concrete types than `T` and `U`. Worth noting are the `transmute` functions for slices and the `from_utf8*` family for mutable slices. Additionally, `mem::transmute` was often used for casting raw pointers, when you can already cast raw pointers just fine with `as`.

This builds upon #27233.
@bors bors merged commit 33af24c into rust-lang:master Aug 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants