-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Make Layout::new const #66254
Make Layout::new const #66254
Conversation
r? @KodrAus (rust_highfive has picked a reviewer for you, use r? to override) |
cc @rust-lang/libs for [needs-fcp] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me!
@rfcbot fcp merge
@rfcbot fcp merge |
Team member @sfackler 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. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. The RFC will be merged soon. |
@bors r+ |
📌 Commit e8c53a6 has been approved by |
🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened |
⌛ Testing commit e8c53a6 with merge ca7e02a1ee5dc9691042789d473f1b574af59be8... |
Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
💔 Test failed - checks-azure |
☔ The latest upstream changes (presumably #67540) made this pull request unmergeable. Please resolve the merge conflicts. |
Unbeknown I did the same thing (and more) in #67494 ; should I wait for this to be rebased and resubmit? |
well this stabilizes it, while your PR just makes it const on nightly. I guess it would be best to wait for this PR and then stabilize the rest. If you want you can already change your PR to just not touch |
1 similar comment
well this stabilizes it, while your PR just makes it const on nightly. I guess it would be best to wait for this PR and then stabilize the rest. If you want you can already change your PR to just not touch |
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.
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.
Rebased again. |
@bors r=KodrAus |
📌 Commit d860def has been approved by |
Make Layout::new const This seems like a reasonable change to make. If we don't provide `Layout::new::<T>` as `const`, then users can just instead do the more error prone `Layout::from_size_align_unchecked(mem::size_of::<T>(), mem::align_of::<T>())` for the same effect and an extra `unsafe { }` incantation.
Rollup of 10 pull requests Successful merges: - #66254 (Make Layout::new const) - #67122 (Do not deduplicate diagnostics in UI tests) - #67358 (Add HashSet::get_or_insert_owned) - #67725 (Simplify into_key_slice_mut) - #67935 (Relax the Sized bounds on Pin::map_unchecked(_mut)) - #67967 (Delay bug to prevent ICE in MIR borrowck) - #67975 (Export public scalar statics in wasm) - #68006 (Recognise riscv64 in compiletest) - #68040 (Cleanup) - #68054 (doc: add Null-unchecked version section to mut pointer as_mut method) Failed merges: - #67258 (Introduce `X..`, `..X`, and `..=X` range patterns) r? @ghost
Tracking issue rust-lang#67521, Layout::new in rust-lang#66254
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()); |
There was a problem hiding this comment.
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!
?
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 fn
s 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.
There was a problem hiding this comment.
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.
Version 1.42.0 (2020-03-12) ========================== Language -------- - [You can now use the slice pattern syntax with subslices.][67712] e.g. ```rust fn foo(words: &[&str]) { match words { ["Hello", "World", "!", ..] => println!("Hello World!"), ["Foo", "Bar", ..] => println!("Baz"), rest => println!("{:?}", rest), } } ``` - [You can now use `#[repr(transparent)]` on univariant `enum`s.][68122] Meaning that you can create an enum that has the exact layout and ABI of the type it contains. - [There are some *syntax-only* changes:][67131] - `default` is syntactically allowed before items in `trait` definitions. - Items in `impl`s (i.e. `const`s, `type`s, and `fn`s) may syntactically leave out their bodies in favor of `;`. - Bounds on associated types in `impl`s are now syntactically allowed (e.g. `type Foo: Ord;`). - `...` (the C-variadic type) may occur syntactically directly as the type of any function parameter. These are still rejected *semantically*, so you will likely receive an error but these changes can be seen and parsed by procedural macros and conditional compilation. Compiler -------- - [Added tier 2* support for `armv7a-none-eabi`.][68253] - [Added tier 2 support for `riscv64gc-unknown-linux-gnu`.][68339] - [`Option::{expect,unwrap}` and `Result::{expect, expect_err, unwrap, unwrap_err}` now produce panic messages pointing to the location where they were called, rather than `core`'s internals. ][67887] * Refer to Rust's [platform support page][forge-platform-support] for more information on Rust's tiered platform support. Libraries --------- - [`iter::Empty<T>` now implements `Send` and `Sync` for any `T`.][68348] - [`Pin::{map_unchecked, map_unchecked_mut}` no longer require the return type to implement `Sized`.][67935] - [`io::Cursor` now derives `PartialEq` and `Eq`.][67233] - [`Layout::new` is now `const`.][66254] - [Added Standard Library support for `riscv64gc-unknown-linux-gnu`.][66899] Stabilized APIs --------------- - [`CondVar::wait_while`] - [`CondVar::wait_timeout_while`] - [`DebugMap::key`] - [`DebugMap::value`] - [`ManuallyDrop::take`] - [`matches!`] - [`ptr::slice_from_raw_parts_mut`] - [`ptr::slice_from_raw_parts`] Cargo ----- - [You no longer need to include `extern crate proc_macro;` to be able to `use proc_macro;` in the `2018` edition.][cargo/7700] Compatibility Notes ------------------- - [`Error::description` has been deprecated, and its use will now produce a warning.][66919] It's recommended to use `Display`/`to_string` instead. - [`use $crate;` inside macros is now a hard error.][37390] The compiler emitted forward compatibility warnings since Rust 1.14.0. - [As previously announced, this release reduces the level of support for 32-bit Apple targets to tier 3.][apple-32bit-drop]. This means that the source code is still available to build, but the targets are no longer tested and no release binary is distributed by the Rust project. Please refer to the linked blog post for more information. [37390]: rust-lang/rust#37390 [68253]: rust-lang/rust#68253 [68348]: rust-lang/rust#68348 [67935]: rust-lang/rust#67935 [68339]: rust-lang/rust#68339 [68122]: rust-lang/rust#68122 [67712]: rust-lang/rust#67712 [67887]: rust-lang/rust#67887 [67131]: rust-lang/rust#67131 [67233]: rust-lang/rust#67233 [66899]: rust-lang/rust#66899 [66919]: rust-lang/rust#66919 [66254]: rust-lang/rust#66254 [cargo/7700]: rust-lang/cargo#7700 [`DebugMap::key`]: https://doc.rust-lang.org/stable/std/fmt/struct.DebugMap.html#method.key [`DebugMap::value`]: https://doc.rust-lang.org/stable/std/fmt/struct.DebugMap.html#method.value [`ManuallyDrop::take`]: https://doc.rust-lang.org/stable/std/mem/struct.ManuallyDrop.html#method.take [`matches!`]: https://doc.rust-lang.org/stable/std/macro.matches.html [`ptr::slice_from_raw_parts_mut`]: https://doc.rust-lang.org/stable/std/ptr/fn.slice_from_raw_parts_mut.html [`ptr::slice_from_raw_parts`]: https://doc.rust-lang.org/stable/std/ptr/fn.slice_from_raw_parts.html [`CondVar::wait_while`]: https://doc.rust-lang.org/stable/std/sync/struct.Condvar.html#method.wait_while [`CondVar::wait_timeout_while`]: https://doc.rust-lang.org/stable/std/sync/struct.Condvar.html#method.wait_timeout_while
This seems like a reasonable change to make. If we don't provide
Layout::new::<T>
asconst
, then users can just instead do the more error proneLayout::from_size_align_unchecked(mem::size_of::<T>(), mem::align_of::<T>())
for the same effect and an extraunsafe { }
incantation.