Skip to content
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

Transmute cleanup #18318

Merged
merged 2 commits into from
Nov 4, 2014
Merged

Transmute cleanup #18318

merged 2 commits into from
Nov 4, 2014

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Oct 25, 2014

clean-up transmutes in librustc and libsyntax

@rust-highfive
Copy link
Contributor

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

index: &mut Vec<entry<i64>>) {
let mut rbml_w = unsafe { rbml_w.unsafe_clone() };
// See above
let ecx: &EncodeContext = unsafe { mem::transmute(ecx_ptr) };
Copy link
Member

Choose a reason for hiding this comment

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

Whoa, so glad to see this gone 😍

@eddyb
Copy link
Member

eddyb commented Oct 26, 2014

LGTM, but there's enough unsafe code here that I'd like a second look.
cc @pcwalton @nikomatsakis @pnkfelix

@@ -798,7 +786,8 @@ fn get_metadata_section_imp(os: abi::Os, filename: &Path) -> Result<MetadataBlob
let csz = llvm::LLVMGetSectionSize(si.llsi) as uint;
let mut found =
Err(format!("metadata not found: '{}'", filename.display()));
let cvbuf: *const u8 = mem::transmute(cbuf);
let cvbuf: *const u8 = mem::transmute::<*const i8,
*const u8>(cbuf);
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't need to be a transmute a simple as cast should work.

@arielb1
Copy link
Contributor Author

arielb1 commented Oct 27, 2014

Some of the transmutes here are DST-related and are waiting for a new snapshot with #17178.

@nikomatsakis
Copy link
Contributor

@arielb1 should the r+ be removed until new snapshot lands?

@arielb1
Copy link
Contributor Author

arielb1 commented Oct 28, 2014

@nikomatsakis

This PR is snapshot-independent. Some further cleanup is waiting on #18291.

@nikomatsakis
Copy link
Contributor

@arielb1 looks like this needs to be rebased

None of them would break by implementation-defined struct layout, but
one would break with strict lifetime aliasing, and the rest are just
ugly code.
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Nov 4, 2014
@bors bors merged commit a87078a into rust-lang:master Nov 4, 2014
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