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

Add {size/min_align/pref_align}_of_val intrinsic #21587

Closed
wants to merge 1 commit into from
Closed

Add {size/min_align/pref_align}_of_val intrinsic #21587

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 24, 2015

Allows DSTs to be sized generically at runtime.

core::mem::{size/align/min_align}_of_val have also been slightly altered
to use the new intrinsics, and to also allow DSTs.

Doesn't work for DST enums and tuples, because I had nothing to test with to ensure it works. Taking a pointer to a DST enum or tuple is currently an ICE(#16812). Would it be better to wait until that ICE is fixed before doing this? Would it be better to add skeleton functionality that should work once the ICE is fixed?

Trait Objects currently seem to only store min_align in their vtables (presuming that this is correct? My tests suggest that the value stored there matched min_align rather than pref_align, at least) so pref_align_of_val will just return that for now. Is that okay?

closes #19063

@rust-highfive
Copy link
Collaborator

r? @pcwalton

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

@@ -79,6 +96,22 @@ pub fn min_align_of<T>() -> uint {
unsafe { intrinsics::min_align_of::<T>() }
}

/// Returns the ABI-required minimum alignment of the type of the value that `val` points to
Copy link
Member

Choose a reason for hiding this comment

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

trailing '.'

@huonw
Copy link
Member

huonw commented Jan 24, 2015

Awesome!

r? @nick29581

@rust-highfive rust-highfive assigned nrc and unassigned pcwalton Jan 24, 2015
Allows DSTs to be sized generically at runtime.

core::mem::{size/align/min_align}_of_val have also been slightly altered
to use the new intrinsics, and to also allow DSTs.

closes #19063
@ghost
Copy link
Author

ghost commented Jan 25, 2015

I realised shortly after doing this that most of what I did was already in glue.rs, so I made some modifications to the function there so that I could use it instead.

There were a couple of errors in the file that I noticed and changed:

  • align was using the lower of the two alignment values, rather than the higher.
  • sized_size was using llsize_of_alloc, which includes padding, which is not what is really wanted.

@ghost ghost closed this Jan 25, 2015
@huonw
Copy link
Member

huonw commented Jan 26, 2015

Hey @whataloadofwhat, did you mean to close this PR?

@ghost
Copy link
Author

ghost commented Jan 26, 2015

Yes, I was talking with eddyb on the irc and realised that it was probably going to be producing wrong results so I closed it.

I'm still looking for a way to get it to work, but I don't expect it to happen any time soon (if at all, I'm a little out of my depth here to be honest), and I don't want to discourage someone else from giving it a try by there being an open pull request for it.

Sorry about that, but thanks for the interest.

This pull request was closed.
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.

mem::{size,align,min_align}_of_val should accept DSTs
4 participants