Skip to content

Conversation

@cgaebel
Copy link

@cgaebel cgaebel commented Oct 25, 2014

Fixes #18302

r? @thestinger

@rust-highfive
Copy link
Contributor

warning Warning warning

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

@thestinger
Copy link
Contributor

@cgaebel: Since Vec<T> doesn't use repr(C), it would technically be undefined behaviour to rely on the on the representation. I still think it's worth having the order match Slice because in practice the compiler is only going to reorder fields when it reduces the size (never these) or if something like randomization is enabled for hardening.

@cgaebel
Copy link
Author

cgaebel commented Oct 25, 2014

Hmmm then would be make sense to mark both raw::Slice and Vec as #[repr(C)] ?

@thestinger
Copy link
Contributor

@cgaebel: I don't think Vec<T> should be repr(C). The Slice type does need to be repr(C) to have a guarantee of matching the compiler representation.

@thestinger
Copy link
Contributor

In the case where struct randomization isn't enabled, matching the representation of slices should allow more optimizations by LLVM. It does make sense to define it in the same order but that order being preserved is not guaranteed by the language.

@cgaebel
Copy link
Author

cgaebel commented Oct 25, 2014

Maybe I'm just not understanding what you're saying, but doesn't #[repr(C)] guarantee the order be preserved?

@thestinger
Copy link
Contributor

It does guarantee that, but tagging stuff as #[repr(C)] when it's not actually needed would make randomization less useful.

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Oct 27, 2014
@bors bors merged commit 87a5f0d into rust-lang:master Oct 28, 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.

Reorder Vec fields

5 participants