Skip to content

Stabilize alloc_layout_extra #97438

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

Closed
wants to merge 2 commits into from
Closed

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented May 26, 2022

Stabilizes #55724. Specifically, stabilizes the functions

// core::alloc
impl Layout {
    pub fn dangling(&self) -> ptr::NonNull<u8>;
    pub fn repeat(&self, n: usize) -> Result<(Self, usize), LayoutError>;
    pub fn repeat_packed(&self, n: usize) -> Result<Self, LayoutError>;
    pub fn extend_packed(&self, next: Self) -> Result<Self, LayoutError>;
}

This PR contains one change beyond stabilization: it changes Layout::repeat to not add trailing padding (i.e. be equivalent to repeated extend calls). This is a functionality change, and should be of special interest during FCP.

If libs-api prefers including adding trailing padding in this API, I can revert that part of this PR. Layout code is notorious for being perf-sensitive to the smallest changes, so probably wants a perf run or at least rollup=never while it includes code changes.


Arguments for trailing padding in Layout::repeat:

  • Matches Layout::array in producing layout suitable for an array field, just with a dynamic Layout rather than a static T layout
  • The layout without trailing padding can be easily acquired via multiple calls to Layout::extend
  • layout.repeat(n)?.0.pad_to_align() subtracts the trailing padding just to add it back on again and the optimizer potentially won't see though that

Arguments against trailing padding in Layout::repeat

  • Matches Layout::extend in not adding trailing padding, just extending the layout as necessary
  • Makes layout.repeat(1) a no-op rather than padding to alignment
  • Adding the trailing padding is easily done via Layout::pad_to_align
  • Getting the layout without trailing padding requires an imperative loop of Layout::extend

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 26, 2022
@rust-highfive
Copy link
Contributor

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Contributor

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 26, 2022
@CAD97
Copy link
Contributor Author

CAD97 commented May 26, 2022

r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs +needs-fcp

@rustbot rustbot added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 26, 2022
@CAD97 CAD97 force-pushed the alloc_layout_extra branch 3 times, most recently from 73feb13 to c855385 Compare May 26, 2022 21:21
@rust-log-analyzer

This comment has been minimized.

@CAD97 CAD97 force-pushed the alloc_layout_extra branch from c855385 to f1ef497 Compare May 26, 2022 22:32
@CAD97 CAD97 force-pushed the alloc_layout_extra branch from f1ef497 to a342c0f Compare May 26, 2022 22:50
@CAD97 CAD97 force-pushed the alloc_layout_extra branch from 02ee8e9 to 37fe087 Compare June 3, 2022 20:30
@CAD97
Copy link
Contributor Author

CAD97 commented Jun 3, 2022

Reverted the Layout::padding_needed_for part of the stabilization, per @scottmcm's comments

@CAD97
Copy link
Contributor Author

CAD97 commented Jun 22, 2022

(triaging my open PRs)

(Trying) Rotating the assignee (it's been a month)

r? rust-lang/libs-api

Requesting I-libs-api-nominated T-libs-api nomination, for 1) FCP, 2) decision on whether Layout::repeat should add trailing padding (to match array layout) or not (to match field layout).

@bors
Copy link
Collaborator

bors commented Jul 10, 2022

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

@est31
Copy link
Member

est31 commented Aug 27, 2022

👋 Hello, I'm writing this comment in this stabilization PR to notify you, the authors of this PR, that #100591 has been merged, which implemented a change in how features are stabilized.

Your PR has been filed before the change, so will likely require modifications in order to comply with the new rules. I recommend you to:

  1. rebase the PR onto latest master, so that uses of the placeholder are possible.
  2. replace the version numbers in the PR with the placeholder CURRENT_RUSTC_VERSION. For language changes, this means the version numbers in accepted.rs (example: 4caedba). For library changes, this means the since fields (example e576a9b).

That's it! The CURRENT_RUSTC_VERSION placeholder will, as part of the release process, be replaced with the version number that the PR merged for. It can be used anywhere in rust-lang/rust, not just accepted.rs and the since fields.

If you have any questions, feel free to drop by the zulip stream, or ping me directly in this PR's thread. Thanks! 👋

@CAD97
Copy link
Contributor Author

CAD97 commented Aug 28, 2022

I'm going to close this PR in favor of opening a new one that just stabilizes the truly nonproblematic dangling and _packed.

@CAD97 CAD97 closed this Aug 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

9 participants