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

Stabilize most common subset of alloc_layout_extras #69362

Open
wants to merge 1 commit into
base: master
from

Conversation

@CAD97
Copy link
Contributor

CAD97 commented Feb 21, 2020

Tracking issue: #55724

Specifically, this stabilizes:

pub fn Layout::align_to(&self, align: usize) -> Result<Layout, LayoutErr>;
pub fn Layout::pad_to_align(&self) -> Layout;
pub fn Layout::extend(&self, next: Layout) -> Result<(Layout, usize), LayoutErr>;
pub fn Layout::array<T>(n: usize) -> Result<Layout, LayoutErr>;

Methods that are tracked by #55724 but are not stabilized here:

pub fn Layout::padding_needed_for(&self, align: usize) -> usize;
pub fn Layout::repeat(&self, n: usize) -> Result<(Layout, usize), LayoutErr>;
pub fn Layout::repeat_packed(&self, n: usize) -> Result<Layout, LayoutErr>;
pub fn Layout::extend_packed(&self, next: Layout) -> Result<Layout, LayoutErr>;

Combined, these stabilized functions allow code to construct and manipulate repr(C) layouts while letting the standard library handle correctness in the face of edge cases. For example use cases, consider the usage in hashbrown, crossbeam-skiplist, pointer-utils/slice-dst, and of course the standard library itself.

Providing a higher-level API such as Layout::repr_c<const N: usize>(fields: [Layout; N]) -> Result<(Layout, [usize; N]), LayoutErr> is blocked on const generics, which are a ways off. Providing an API that doesn't provide offsets would be quite suboptimal, as the reason for calculating the layout like this rather than Layout::new is to get the field offsets.

The primary issue with the current API is having to call .pad_to_align() to match the layout of a repr(C) struct. However, I think this is not just a (failing? limitation?) of the API, but rather intrinsic complexity. While all Rust-defined types have size==stride, and probably will for the foreseeable future, there is no inherent reason why this is a limitation of all allocations. As such, the Layout manipulation APIs shouldn't impose this limitation, and instead the higher level api of repr_c (or just plain old using Layout::new) can make keeping it simple.

cc @matklad r? @rust-lang/libs

@CAD97

This comment has been minimized.

Copy link
Contributor Author

CAD97 commented Feb 21, 2020

whoops I broke highfive by r-ing a team, so rng says...

r? @dtolnay

@Centril Centril added this to the 1.43 milestone Feb 21, 2020
@Amanieu

This comment has been minimized.

Copy link
Contributor

Amanieu commented Feb 21, 2020

I would like Layout::repeat to be stabilized as well, it is a very common operation when dealing with complex allocations.

@CAD97

This comment has been minimized.

Copy link
Contributor Author

CAD97 commented Feb 22, 2020

Personally, I'm not comfortable making the call on whether repeat is correctly specified or not. @mooli brought up that repeat includes trailing padding, which is arguably not necessary.

In another formulation, layout.repeat(n) produces a distinct result from calling extend multiple times. I've actually added an unnecessary-with-current-repeat pad_to_align call to array here (which, in hindsight, is probably not necessary even with a weaker repeat that doesn't provide trailing padding because size==stride).

I don't feel confident calling for the stabilization of repeat without addressing the difference between repeat and multiple calls to extend, whereas the four I've called for here are much more obviously correct.

@Amanieu

This comment has been minimized.

Copy link
Contributor

Amanieu commented Feb 22, 2020

Fair enough, let's defer on repeat for now then.

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 22, 2020

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@CAD97 CAD97 force-pushed the CAD97:alloc_layout_extras branch from 1bf23b7 to 6d9ecb2 Mar 27, 2020
@CAD97

This comment has been minimized.

Copy link
Contributor Author

CAD97 commented Mar 27, 2020

Amended to refer to a 1.44 stabilization in the attributes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.