Uh oh!
There was an error while loading. Please reload this page.
Add tracking issue for Layout methods (and some API changes) - #55366
Conversation
Uh oh!
There was an error while loading. Please reload this page.
SimonSapin
commented
Oct 26, 2018
… but This seems inconsistent for no reason, should we change |
SimonSapin
commented
Oct 26, 2018
@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. |
SimonSapin
commented
Oct 26, 2018
@rfcbot concern return types consistency For #55366 (comment) |
SimonSapin
commented
Oct 26, 2018
@rfcbot concern compat with repr(C) For example: #[repr(C)]structPair<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 |
Centril
commented
Oct 26, 2018
cc @rust-lang/lang |
Amanieu
commented
Oct 26, 2018
Yes this will always hold because the behavior of |
SimonSapin
commented
Oct 27, 2018
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? |
Amanieu
commented
Oct 27, 2018
I feel that we definitely need to change something. My preference would be to change |
SimonSapin
commented
Oct 28, 2018
alexcrichton
commented
Oct 29, 2018
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:
|
Amanieu
commented
Oct 29, 2018
Regarding |
SimonSapin
commented
Oct 29, 2018
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 |
alexcrichton
commented
Oct 30, 2018
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 |
Amanieu
commented
Oct 30, 2018
@alexcrichton I personally feel that the For |
alexcrichton
commented
Nov 1, 2018
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 |
SimonSapin
commented
Nov 1, 2018
I think that
I don’t know when |
Amanieu
commented
Nov 1, 2018
I agree with @SimonSapin here: For What are you thoughts on stabilizing |
alexcrichton
commented
Nov 5, 2018
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
commented
Nov 6, 2018
I have changed the PR to just split the |
alexcrichton
commented
Nov 6, 2018
Thanks for the update! r=me but it looks like there's some minor build failures @bors: delegate+ |
bors
commented
Nov 6, 2018
✌️ @Amanieu can now approve this pull request |
Amanieu
commented
Nov 6, 2018
@bors: r+ |
bors
commented
Nov 6, 2018
📌 Commit 02d50de has been approved by |
bors
commented
Nov 7, 2018
⌛ Testing commit 02d50de with merge a22c88419fde9fd1845b994b4dcd4577e2f3bcfc... |
bors
commented
Nov 7, 2018
💔 Test failed - status-travis |
rust-highfive
commented
Nov 7, 2018
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 |
Amanieu
commented
Nov 7, 2018
The failure seems to be unrelated to this PR? @bors retry |
bors
commented
Nov 8, 2018
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
bors
commented
Nov 8, 2018
☀️ Test successful - status-appveyor, status-travis |
rust-highfive
commented
Nov 8, 2018
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).
kennytm
commented
Nov 8, 2018
The nomicon failure: |
These methods are already useful when used with the stable global allocator API (stabilized in #51241).
cc #32838
r? @SimonSapin