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

Merged
merged 7 commits into from
Apr 21, 2020

Conversation

CAD97
Copy link
Contributor

@CAD97 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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 21, 2020
@CAD97
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 relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Feb 21, 2020
@Centril Centril added this to the 1.43 milestone Feb 21, 2020
@Amanieu
Copy link
Member

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
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
Copy link
Member

Amanieu commented Feb 22, 2020

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

@rfcbot fcp merge

@rfcbot
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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Feb 22, 2020
@Centril Centril modified the milestones: 1.43, 1.44 Mar 10, 2020
@CAD97
Copy link
Contributor Author

CAD97 commented Mar 27, 2020

Amended to refer to a 1.44 stabilization in the attributes.

@bors
Copy link
Contributor

bors commented Apr 2, 2020

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

@Amanieu
Copy link
Member

Amanieu commented Apr 2, 2020

ping @sfackler @KodrAus @withoutboats for checkboxes.

1 similar comment
@Amanieu
Copy link
Member

Amanieu commented Apr 7, 2020

ping @sfackler @KodrAus @withoutboats for checkboxes.

@CAD97
Copy link
Contributor Author

CAD97 commented Apr 8, 2020

Rebased.

@Amanieu
Copy link
Member

Amanieu commented Apr 10, 2020

(I went ahead and checked for @KodrAus since he's busy)

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Apr 10, 2020
@rfcbot
Copy link

rfcbot commented Apr 10, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 20, 2020
@rfcbot
Copy link

rfcbot commented Apr 20, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

Copy link
Member

@Amanieu Amanieu left a comment

Choose a reason for hiding this comment

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

r=me after fixing the doc comments.

src/libcore/alloc/layout.rs Outdated Show resolved Hide resolved
src/libcore/alloc/layout.rs Outdated Show resolved Hide resolved
@Amanieu
Copy link
Member

Amanieu commented Apr 20, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Apr 20, 2020

📌 Commit 98f0a82 has been approved by Amanieu

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 20, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 21, 2020
Rollup of 4 pull requests

Successful merges:

 - rust-lang#69362 (Stabilize most common subset of alloc_layout_extras)
 - rust-lang#71174 (Check that main/start is not async)
 - rust-lang#71285 (MIR: use HirId instead of NodeId to avoid cycles while inlining)
 - rust-lang#71346 (Do not build tools if user do not want them)

Failed merges:

r? @ghost
@bors bors merged commit 69a528e into rust-lang:master Apr 21, 2020
Aaron1011 added a commit to Aaron1011/blog_os that referenced this pull request May 19, 2020
A subset of this feature was stabilized in rust-lang/rust#69362,
and none of the still-unstable methods are in use in `blog_os`
phil-opp pushed a commit to phil-opp/blog_os that referenced this pull request May 20, 2020
A subset of this feature was stabilized in rust-lang/rust#69362,
and none of the still-unstable methods are in use in `blog_os`
@CAD97 CAD97 deleted the alloc_layout_extras branch November 6, 2023 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants