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

Constify more of alloc::Layout #67494

Merged
merged 1 commit into from Jan 12, 2020
Merged

Constify more of alloc::Layout #67494

merged 1 commit into from Jan 12, 2020

Conversation

@lukaslueg
Copy link
Contributor

lukaslueg commented Dec 21, 2019

Making more methods of alloc::Layout const would allow us to compute alignment/size information for arbitrary (sized) types at compile-time, including placing the information in associated constants. While mem::size_of and mem::align_of are already const and Layout is solely based on those, there is no guarantee in the implementation that a const derived from these functions will be exactly the same as what Layout uses and is subsequently used in a call to alloc::alloc. Constifying Layout makes this possible.

First contribution to core, excuse my ignorance.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 21, 2019

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Dec 21, 2019

Copy link
Contributor

oli-obk left a comment

Please file a tracking issue for this constification and use the id instead of "0"

src/libcore/alloc.rs Outdated Show resolved Hide resolved
src/libcore/alloc.rs Outdated Show resolved Hide resolved
src/libcore/alloc.rs Outdated Show resolved Hide resolved
@lukaslueg

This comment has been minimized.

Copy link
Contributor Author

lukaslueg commented Dec 22, 2019

I split the consttransmogrification of Result into a separate feature, I hope this was ok.

pub fn align_to(&self, align: usize) -> Result<Self, LayoutErr> {
Layout::from_size_align(self.size(), cmp::max(self.align(), align))
pub const fn align_to(&self, align: usize) -> Result<Self, LayoutErr> {
let align = if self.align() >= align { self.align() } else { align };

This comment has been minimized.

Copy link
@oli-obk

oli-obk Dec 22, 2019

Contributor

Please make cmp::max const fn, too :)

This comment has been minimized.

Copy link
@lukaslueg

lukaslueg Dec 22, 2019

Author Contributor

Should I file a separate tracking issue?

This comment has been minimized.

Copy link
@oli-obk

oli-obk Dec 22, 2019

Contributor

sorry I missed this in the first pass

the PR lgtm after this

This comment has been minimized.

Copy link
@lukaslueg

lukaslueg Dec 22, 2019

Author Contributor

Oh I just realized: std::cmp::{max; min} are defined as pub fn max<T>(v1: T, v2: T) -> T where T: Ord. There is no way to require an impl of Ord to be const?!

This comment has been minimized.

Copy link
@oli-obk

oli-obk Dec 22, 2019

Contributor

Oh, no that's not possible. So leave align_to as not const fn for now, or is it super necessary?

This comment has been minimized.

Copy link
@lukaslueg

lukaslueg Dec 22, 2019

Author Contributor

Not to me, no. I actually don't see this one-liner as a problem: The type is always going to be usize, so even if it never gets changed back to cmp::max(), it's totally obvious what it does.

This comment has been minimized.

Copy link
@oli-obk

oli-obk Dec 22, 2019

Contributor

it's totally obvious what it does.

I agree, but

  • the line of what is obvious and what not is very subjective, if at some point we have to start arguing about it, it seems good to start out conservative if there's no strong desire for the main change
  • the original author must have thought that cmp::max is even more obvious (and I think this, too). So making code less obvious just to make it const without a strong reason to do so seems not like a good strategy.

Thanks for changing it back

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Dec 22, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 22, 2019

📌 Commit b56e0e2 has been approved by oli-obk

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 22, 2019

🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened

@lukaslueg

This comment has been minimized.

Copy link
Contributor Author

lukaslueg commented Dec 22, 2019

I've just seen this will conflict with #66254 which does constification for just new. You may want to r- ?

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Dec 22, 2019

@bors r-

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 23, 2019

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

This was referenced Dec 25, 2019
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Dec 31, 2019
Constify Result

r? @oli-obk

This is just the `Result`-part of rust-lang#67494 which I'll resubmit once rust-lang#66254 has landed.
Centril added a commit to Centril/rust that referenced this pull request Dec 31, 2019
Constify Result

r? @oli-obk

This is just the `Result`-part of rust-lang#67494 which I'll resubmit once rust-lang#66254 has landed.
@lukaslueg lukaslueg force-pushed the lukaslueg:const_alloc branch from b56e0e2 to cf89869 Dec 31, 2019
@lukaslueg

This comment has been minimized.

Copy link
Contributor Author

lukaslueg commented Dec 31, 2019

Force-pushed a commit that wont conflict with #66254

@CAD97

This comment has been minimized.

Copy link
Contributor

CAD97 commented Jan 9, 2020

wont conflict with #66254

Actually, since we both (necessarily) touch size_align to make it const. You could rebase on top of #66254 or just wait for it to get merged.

@lukaslueg lukaslueg force-pushed the lukaslueg:const_alloc branch from cf89869 to c5ac45e Jan 10, 2020
@lukaslueg

This comment has been minimized.

Copy link
Contributor Author

lukaslueg commented Jan 10, 2020

rebased again

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 11, 2020

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

Tracking issue #67521, Layout::new in #66254
@lukaslueg lukaslueg force-pushed the lukaslueg:const_alloc branch from c5ac45e to c5a9a14 Jan 11, 2020
@lukaslueg

This comment has been minimized.

Copy link
Contributor Author

lukaslueg commented Jan 11, 2020

rebased again

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Jan 11, 2020

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 11, 2020

📌 Commit c5a9a14 has been approved by oli-obk

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 12, 2020

⌛️ Testing commit c5a9a14 with merge eb9af48...

bors added a commit that referenced this pull request Jan 12, 2020
Constify more of alloc::Layout

Making more methods of `alloc::Layout` const would allow us to compute alignment/size information for arbitrary (sized) types at compile-time, including placing the information in associated constants. While `mem::size_of` and `mem::align_of` are already const and `Layout` is solely based on those, there is no guarantee in the implementation that a const derived from these functions will be exactly the same as what `Layout` uses and is subsequently used in a call to `alloc::alloc`. Constifying `Layout` makes this possible.

First contribution to core, excuse my ignorance.
Centril added a commit to Centril/rust that referenced this pull request Jan 12, 2020
Constify more of alloc::Layout

Making more methods of `alloc::Layout` const would allow us to compute alignment/size information for arbitrary (sized) types at compile-time, including placing the information in associated constants. While `mem::size_of` and `mem::align_of` are already const and `Layout` is solely based on those, there is no guarantee in the implementation that a const derived from these functions will be exactly the same as what `Layout` uses and is subsequently used in a call to `alloc::alloc`. Constifying `Layout` makes this possible.

First contribution to core, excuse my ignorance.
@Centril

This comment has been minimized.

Copy link
Member

Centril commented Jan 12, 2020

@bors retry rolled up.

bors added a commit that referenced this pull request Jan 12, 2020
Rollup of 6 pull requests

Successful merges:

 - #67494 (Constify more of alloc::Layout)
 - #67867 (Correctly check for opaque types in `assoc_ty_def`)
 - #67948 (Galloping search for binary_search_util)
 - #68045 (Move more of `rustc::lint` into `rustc_lint`)
 - #68089 (Unstabilize `Vec::remove_item`)
 - #68108 (Add suggestions when encountering chained comparisons)

Failed merges:

r? @ghost
@bors bors merged commit c5a9a14 into rust-lang:master Jan 12, 2020
4 of 5 checks passed
4 of 5 checks passed
homu Testing commit c5a9a14c9f793ce20d34cffac09f4ee21d541565 with merge eb9af48328fc6dee800ed3fe2f55551322a09cd6...
Details
pr Build #20200111.50 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@lukaslueg lukaslueg deleted the lukaslueg:const_alloc branch Jan 12, 2020
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

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