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

Jump to bottom

Make Layout::new const #66254

Merged
merged 2 commits into from Jan 9, 2020
Merged

Make Layout::new const #66254

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e47fec5
Make Layout::new const
CAD97 Nov 9, 2019
d860def
Mark Layout::new as const stable
CAD97 Dec 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/libcore/alloc.rs
View file
Open in desktop
Expand Up @@ -17,7 +17,7 @@ use crate::usize;
#[derive(Debug)]
pub struct Excess(pub NonNull<u8>, pub usize);

fn size_align<T>() -> (usize, usize) {
const fn size_align<T>() -> (usize, usize) {
(mem::size_of::<T>(), mem::align_of::<T>())
}

Expand Down Expand Up @@ -120,14 +120,14 @@ impl Layout {

/// Constructs a `Layout` suitable for holding a value of type `T`.
#[stable(feature = "alloc_layout", since = "1.28.0")]
#[rustc_const_stable(feature = "alloc_layout_const_new", since = "1.42.0")]
#[inline]
pub fn new<T>() -> Self {
pub const fn new<T>() -> Self {
let (size, align) = size_align::<T>();
// Note that the align is guaranteed by rustc to be a power of two and
// the size+align combo is guaranteed to fit in our address space. As a
// result use the unchecked constructor here to avoid inserting code
// that panics if it isn't optimized well enough.
debug_assert!(Layout::from_size_align(size, align).is_ok());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have conditionals in const, can we re-add the debug_assert!?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the stdlib is always compiled in release mode, it's not too useful to re-add.

But yes, given feature(const_fn, const_if_match, and const_panic), this debug assert can be made to compile. (Ideally it'd also have const fn Result::is_ok to avoid using matches! directly.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have some CI runners that have debug assertions enabled; it is not uncommon for rustc devs to enable debug assertions locally; Miri will hopefully enable libstd debug assertions eventually. So, there is some use to these (and the libstd is using them in plenty of places).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_ok was recently made const in #67685

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RalfJung For some reason, while I can get debug_assert! in const_fn on the playground, any attempt to add it in libcore, even #[cfg(not(bootstrap))] is giving me error[E0723]: loops and conditional expressions are not stable in const fn.

diff --git a/src/libcore/alloc.rs b/src/libcore/alloc.rs
index a04e75bc7ce..a679fe160a8 100644
--- a/src/libcore/alloc.rs
+++ b/src/libcore/alloc.rs
@@ -131,6 +131,8 @@ impl Layout {
         // the size+align combo is guaranteed to fit in our address space. As a
         // result use the unchecked constructor here to avoid inserting code
         // that panics if it isn't optimized well enough.
+        #[cfg(not(bootstrap))]
+        debug_assert!(Layout::from_size_align(size, align).is_ok());
         unsafe { Layout::from_size_align_unchecked(size, align) }
     }
 
diff --git a/src/libcore/lib.rs b/src/libcore/lib.rs
index bca96b77812..00ddbb923e2 100644
--- a/src/libcore/lib.rs
+++ b/src/libcore/lib.rs
@@ -82,6 +82,7 @@
 #![feature(const_int_pow)]
 #![feature(constctlz)]
 #![feature(const_panic)]
+#![feature(const_fn)]
 #![feature(const_fn_union)]
 #![feature(const_generics)]
 #![feature(const_ptr_offset_from)]
error[E0723]: loops and conditional expressions are not stable in const fn                                                                    
   --> src\libcore\macros\mod.rs:184:23
    |
183 | / macro_rules! debug_assert {
184 | |     ($($arg:tt)*) => (if $crate::cfg!(debug_assertions) { $crate::assert!($($arg)*); })
    | |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
185 | | }
    | |_- in this expansion of `debug_assert!`
    | 
   ::: src\libcore\alloc.rs:135:9
    |
135 |           debug_assert!(Layout::from_size_align(size, align).is_ok());
    |           ------------------------------------------------------------ in this macro invocation
    |
    = note: see issue #57563 <https://github.com/rust-lang/rust/issues/57563> for more information
    = help: add `#![feature(const_fn)]` to the crate attributes to enable

Copy link
Member

@RalfJung RalfJung Feb 21, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CAD97 Stable const fn cannot use unstable features. There's allow_internal_unstable to work around this, but at this point we might not yet want to stabilize methods that use these features internally. OTOH this is just a debug assertion so it seems fine to me.

Also @oli-obk is the better person for such questions. :) Or just ping the entire team: @rust-lang/wg-const-eval

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually looks like we already have stable const fn that opt into using conditionals, so just adding #[allow_internal_unstable(const_if_match)] should be fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not fine. The stable const fns that use conditionals are just refactorings of hacks that were written without conditionals before, but they do not use more expressive power than what is available on stable, which, as far as I can tell, this would.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would just use it for a debug assertion though, which we can remove again any time if we have to.

unsafe { Layout::from_size_align_unchecked(size, align) }
}

Expand Down