Open lukaslueg opened 4 years ago
Shouldn't most methods of Layout
be const
?
This would allow certain usages where a custom derive + const is used to calculate field offsets of repr(C)
structs at compiler time. Which can be useful to create pointers to fields without ever having a reference to the struct. Which should be useful if working with unaligned structs or creation of custom DST (once we have a way to create fat-pointers by hand). There are probably other usages, too I think.
So following additional methods should become const too in the future in my opinion:
method | hindered by | can be resolved from >1.47.0 on | note |
---|---|---|---|
align_to |
cmp::max::<usize> |
yes | why was it crossed out above? |
pad_to_align |
Result.unwrap() |
yes | The unsafe Layout constructor should be usable but I need to double check this |
repeat |
usize.checked_mul + Option.ok_or + ? |
yes | unsatable |
extend |
cmp::max::<usize> + usize.checked_add + Option.ok_or + ? |
yes | - |
repeat_packed |
usize.checked_mul + Option.ok_or + ? |
yes | unstable |
extend_packed |
checked_add + Option.ok_or + ? |
yes | unstable |
array |
debug_assert_eq! + repeat + ? + pad_to_align |
? | - |
The problems can be resolved as following
problem | solutions | note |
---|---|---|
cmp::max::<usize> |
internal helper const fn align_max(align1: usize, algin2: usize) { ... } |
max is generic and I'm not sure if we can have something like const specialization but a internal const helper function does the job, too |
usize.checked_mul |
stable on beta of 1.47 | - |
usize.checked_add |
stable on beta of 1.47 | - |
Result.unwrap() |
in pad_to_align we probably can use the unsafe constructor of Layout safely |
|
Option.ok_or |
method can be const implemented in the beta of 1.47, | alternative a temporary helper method/explicit match could be used to stabilize const here independent of const methods on Option |
? |
at least from 1.47 one we can use a manual match + early return | see below |
debug_assert_eq! |
isn't really needed in array tbh |
do we have a "debug_assert if we don't run const evaluation method?" |
The main hindrance I see is that not using ?
would make the code more verbose.
All places where it's used do not do any error conversion so all of them can easily
be done in const
(at least from 1.47 onward, maybe earlier). So it mostly boils down to
a match + early return still in extend
half of the code lines use ?
. We could temporary
have a const_try
macro or similar. Best probably would be to allow ?
usage in const use-cases where no type conversion is done e.g. option.ok_or(LayoutErr { private: {} })?
. Most usages of ?
are a checked add/mull followed by a ok_or
mapping and a ?
. Given that we maybe can temporary be ok with a helper macro like bail_on_overflow!{ }
. E.g. for extend
:
pub fn extend(&self, next: Self) -> Result<(Self, usize), LayoutErr> {
let new_align = max_align(self.align(), next.align());
let pad = self.padding_needed_for(next.align());
let offset = bail_on_overflow!{ self.size().checked_add(pad) };
let new_size = bail_on_overflow!{ offset.checked_add(next.size()) };
match Layout::from_size_align(new_size, new_align) {
Ok(layout) => Ok((layout, offset)),
other => other
}
}
///------------- temporary helpers to be used by `extend` and other `Layout` methods
macro_rules! bail_on_overflow!{
($expr:expr) => ({
match $expr {
None => return Err(LayoutErr { private: () }),
Some(x) => x
}
});
}
const fn max_align(align1: usize, align2: usize) -> usize {
if align1 < align2 { align2 }
else { align1 }
}
Not super nice but it would do for now I think.
Lastly let's try to reason why pad_to_algin
should be able to use the unsafe
constructor instead of unwrap()
(and with that can be made const). Constraints for Layout::from_size_align_unchecked
:
align
!= 0align
power of 2size
when rounded up to the nearest multiple of align
must not overflowAnd:
pub fn pad_to_align(&self) -> Layout {
let pad = self.padding_needed_for(self.align());
let new_size = self.size() + pad;
Layout::from_size_align(new_size, self.align()).unwrap()
}
As the original source code already mentions new_size
can not overflow as
pad
is just "rounding" it up the the next multiple of self.align()
. But if that is
the case then new_size
will always be valid afterward as it will be a multiple of
it's alignment and as such the "nearest multiple of align
" is it self. Furthermore
algin is not touched and was valid before.
So following should be fine:
pub const fn pad_to_align(&self) -> Layout {
let pad = self.padding_needed_for(self.align());
// [...]
let new_size = self.size() + pad;
// Safe: [...]
unsafe { Layout::from_size_align_unchecked(new_size, align) }
}
Open questions:
?
/Option.ok_or
?debug_assert_eq!
in array
?It looks like this only has padding_needed_for
left? Can that be stabilized?
The following stable functions are unstably const under this issues:
fn for_value<T>(t: &T) -> Layout
fn align_to(&self, align: usize) -> Result<Layout, LayoutError>
fn pad_to_align(&self) -> Layout
fn extend(&self, next: Layout) -> Result<(Layout, usize), LayoutError>
fn array<T>(n: usize) -> Result<Layout, LayoutError>
The following are unstable functions that are also unstably const:
unsafe fn for_value_raw<T>(t: *const T) -> Layout
fn padding_needed_for(&self, align: usize) -> usize
fn repeat(&self, n: usize) -> Result<(Layout, usize), LayoutError>
fn repeat_packed(&self, n: usize) -> Result<Layout, LayoutError>
fn extend_packed(&self, next: Layout) -> Result<Layout, LayoutError>
I think the unstable functions should simply be const without a separate const unstable issue? const fn
is now in a good enough shape that I believe a decision on whether or not to constify a Layout
function can be made a the same time as stabilizing the function itself. There's no need to track it separately.
Shouldn't padding_need_for
use the Alignment
type rather than a raw usize
?
Then it would be guaranteed that the align
parameter is a strictly positive power-of-2.
Shouldn't padding_need_for use the Alignment type rather than a raw usize?
This is about the constness of these functions, not any other aspect of their API -- that is tracked at https://github.com/rust-lang/rust/issues/55724.
I think the unstable functions should simply be const without a separate const unstable issue? const fn is now in a good enough shape that I believe a decision on whether or not to constify a Layout function can be made a the same time as stabilizing the function itself. There's no need to track it separately.
Yes, good point. I am implementing that in https://github.com/rust-lang/rust/pull/132455.
@rust-lang/libs-api the remaining const fn
tracked here are all already stable. So I suggest we const-stabilize them as well:
impl Layout {
fn for_value<T>(t: &T) -> Layout
fn align_to(&self, align: usize) -> Result<Layout, LayoutError>
fn pad_to_align(&self) -> Layout
fn extend(&self, next: Layout) -> Result<(Layout, usize), LayoutError>
fn array<T>(n: usize) -> Result<Layout, LayoutError>
}
for_value
is blocked on size/align_of_val
, which passed FCP in https://github.com/rust-lang/rust/issues/46571. I'm waiting for confirmation that this won't interact poorly with other ongoing efforts; worst-case we can split that function out and stabilize the rest. So I don't think this has to block FCP here.
@rfcbot fcp merge
Team member @dtolnay 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.
Feature
const_alloc_layout
makes these methods onLayout
const:Making more methods of
alloc::Layout
const allows computing alignment/size information for arbitrary (sized) types at compile-time. Whilemem::size_of
andmem::align_of
are already const andLayout
is solely based on those, there is no guarantee that a const derived from these functions will be exactly the same as what is used in a call toalloc::alloc
. ConstifyingLayout
makes this possible.Implemented in #67494
Blocked on https://github.com/rust-lang/rust/issues/46571 (needed for
for_value
)