Add tracking issue for Layout methods (and some API changes) #55366
Conversation
rust-highfive
added
the
S-waiting-on-review
Amanieu
force-pushed
the
stable_layout
branch
from
74e2fa5 to
2653b60
Compare
Centril
added
the
relnotes
|
… but This seems inconsistent for no reason, should we change |
SimonSapin
added
the
T-libs-api
|
@rfcbot fcp merge |
|
Team member @SimonSapin has proposed to merge this. The next step is review by the rest of the tagged teams: Concerns:
Once a majority of reviewers approve (and none object), 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
added
proposed-final-comment-period
|
@rfcbot concern return types consistency For #55366 (comment) |
|
@rfcbot concern compat with repr(C) For example: #[repr(C)]
struct Pair<A, B> {
a: A,
b: B,
}
assert_eq!(Layout::new::<Pair<Foo, Bar>>(),
Layout::new::<Foo>().extend(Layout::new::<Bar>()))Does this hold for any types Similarly for Note that |
cc @rust-lang/lang |
Yes this will always hold because the behavior of |
|
I suspected that but wasn’t sure how C struct layout is defined. Could you add sentence or two about this in the doc-comments of the relevant methods? |
I feel that we definitely need to change something. My preference would be to change |
|
I'm personally a bit wary to simply stabilize everything as-is as these afaik haven't gone through a great deal of review. I'd rather stabilize methods which have use cases today and have clearly defined signatures and such. For example this PR doesn't itself include motivation (or links to) for stabilization, what's the main use case for stabilizing all of these methods? Some thoughts I have on these methods:
I'm personally would prefer to lean on the side of being conservative due to how low level these are. I'd be fine changing signatures so long as we have good reasons as well! |
|
The main use case is to construct complex allocation layouts for use with the stable global allocator API. For example:
|
|
Regarding |
It sounds like we’re on a good start to do the review in this thread :) And if we’re still unsure about some some methods we can leave them for now and stabilize the rest.
I’d expect the offsets not to be stored. (Growing |
|
I suppose I'm not really comfortable myself proposing stablization without motivation mentioned in the description and also doing an API review at the same time. The Is there a way perhaps the |
|
@alexcrichton I personally feel that the For |
|
Ok well at the very least, I would want to restrict a consideration of stabilization to only I'm still not sure what to do about |
|
I think that
I don’t know when |
|
I agree with @SimonSapin here: For What are you thoughts on stabilizing |
|
FWIW these sorts of comments are what I would expect to be, for example, in the description of this PR, the description of an FCP to stabilize, etc. Right now these comments are buried in a discussion that is quite long at this point and hard to extract, but the comments provide good rationale for stabilizing these methods. Procedurally I would prefer at this point if we open a dedicated tracking issue for the proposal to stabilize here. Such a tracking issue would contain rationale, use cases, etc, along with the proposed-to-be-stabilized interface. Other issues mentioning these APIs could then also get a comment indicating that this tracking issue is proposing stabilization of a few APIs. I'm gonna go ahead and register a formal concern for this PR: @rfcbot concern not-ready-yet |
Amanieu
changed the title
|
I have changed the PR to just split the |
|
Thanks for the update! r=me but it looks like there's some minor build failures @bors: delegate+ |
|
✌️ @Amanieu can now approve this pull request |
Amanieu
force-pushed
the
stable_layout
branch
from
aa05b90 to
02d50de
Compare
|
@bors: r+ |
|
📌 Commit 02d50de has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 02d50de with merge a22c88419fde9fd1845b994b4dcd4577e2f3bcfc... |
|
💔 Test failed - status-travis |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
added
S-waiting-on-review
|
The failure seems to be unrelated to this PR? @bors retry |
bors
added
S-waiting-on-bors
Add tracking issue for Layout methods (and some API changes) These methods are already useful when used with the stable global allocator API (stabilized in #51241). ```rust pub fn align_to(&self, align: usize) -> Result<Layout, LayoutErr>; pub fn padding_needed_for(&self, align: usize) -> usize; pub fn repeat(&self, n: usize) -> Result<(Layout, usize), LayoutErr>; pub fn extend(&self, next: Layout) -> Result<(Layout, usize), LayoutErr>; pub fn repeat_packed(&self, n: usize) -> Result<Layout, LayoutErr>; pub fn extend_packed(&self, next: Layout) -> Result<Layout, LayoutErr>; pub fn array<T>(n: usize) -> Result<Layout, LayoutErr>; ``` cc #32838 r? @SimonSapin
|
☀️ Test successful - status-appveyor, status-travis |
rfcbot
removed
proposed-final-comment-period
Tested on commit rust-lang/rust@1d83455. Direct link to PR: <rust-lang/rust#55366> 💔 nomicon on windows: test-pass → test-fail (cc @frewsxcv @gankro, @rust-lang/infra). 💔 nomicon on linux: test-pass → test-fail (cc @frewsxcv @gankro, @rust-lang/infra).
|
The nomicon failure: |
These methods are already useful when used with the stable global allocator API (stabilized in #51241).
cc #32838
r? @SimonSapin