-
Notifications
You must be signed in to change notification settings - Fork 13k
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 MaybeUninit::assume_init()
a const fn
#66579
Make MaybeUninit::assume_init()
a const fn
#66579
Conversation
It can be, because it simply calls an intrinsics and then another function which is already a `const fn` (`ManuallyDrop::into_inner` specifically.) Example use case for this: https://github.com/slightlyoutofphase/staticvec/blob/addf203c0ec11905c403d25faacfdd29f9191a28/src/lib.rs#L50 If `assume_init` were `const fn`, then the linked `new` function could be also.
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
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 |
Did not know it mattered TBH!
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 |
Hopefully this works. Based on the following playground link, panic_if_uninhabited *is* already valid in `const fn`, to be clear: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=8cbb9b4778e8f93ee1863c64995d15a7
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 |
Definitely confused about why `libcore` can't already use fully itself though.
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 |
Alright, I don't get it. Am I seeing some kind of weird attribute bug here?
|
For reference, here is the entirety of In this form, |
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 |
Alright, not gonna clog up the build queue anymore. Should I open a bug for this, though, or is there something else I'm missing here? Does it maybe have something to do with the fact that |
The behavior you are seeing is here is by design and not a bug. The difference between the playground example and the actual libcore is that you've omitted
For the time being, you can use |
Ah, yeah, it would have to be that way wouldn't it? Sorry for my clumsy attempts so far. Pretty sure I can add a test for it a bit later also, if that's something that would generally be wanted. |
Hmm, I'm not sure this will work as-is. Trying to actually use it while writing a test I get "calling intrinsic It's unclear if that's meant literally though, because looking into it further, it seems like that message is just a catch-all for anything not present in librustc_mir/interpret/intrinsics.rs in the Miri seems to have a version of it that looks compatible (or at least that it could be made compatible) with the code in librustc_mir, though: |
I am not a big fan of letting const code do the equivalent of |
See #66275: intrinsics need to be specifically support in |
To elaborate on my earlier post: at the current state, I don't view "because we can" as sufficient motivation for making tricky operations like The one use-case given is already a hack. This is just an unstable experiment, so not a terribly big deal, but I'd rather we block stabilizing For pub struct StaticVec<T, const N: usize> {
data: MaybeUninit<[MaybeUninit<T>; N]>,
length: usize,
} |
Possibly. I'm not sure how much that would complicate the rest of the crate though. Might not be worth it just to support Anyways, seems like there's generally more complexity to this overall issue than I thought. Should I just close the PR for now, then? It's not that important to me anyways, I can definitely just wait for a better way of doing it that's what you think is a better way to go. I might ask though if there's any aspect of the situation that does need specific work in some area that no one is really working on at the moment? Would be happy to try to help that way if so. |
For array repeat expressions, the tracking issue is #49147. @ecstatic-morse would know the details, but I don't think it's implementation work blocking this right now. For const |
Ok, well I'll close this for now then I guess, and keep an eye on the PRs / issues for anything I think I'd be capable of helping with. |
It can be, because it simply calls an intrinsic and then another function which is already a
const fn
(ManuallyDrop::into_inner
specifically.)Example use case for this:
https://github.com/slightlyoutofphase/staticvec/blob/addf203c0ec11905c403d25faacfdd29f9191a28/src/lib.rs#L50
If
assume_init
wereconst fn
, then the linkednew
function could be also.