Skip to content

[Experiment] Replace unreachable_unchecked() with uninit().assume_init() in unwrap_unchecked() #124737

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

[Experiment] Replace unreachable_unchecked() with uninit().assume_init() in unwrap_unchecked() #124737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3d67c90
Replace `unreachable_unchecked` with `uninit().assume_init()`
alion02 May 5, 2024
b0eca0b
Fix build
alion02 May 5, 2024
7456fd1
Really fix build (I think)
workingjubilee May 5, 2024
fd98988
Really really fix build
workingjubilee May 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions library/core/src/option.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,7 +557,7 @@ use crate::iter::{self, FusedIterator, TrustedLen};
use crate::panicking::{panic, panic_display};
use crate::pin::Pin;
use crate::{
cmp, convert, hint, mem,
cmp, convert, mem,
ops::{self, ControlFlow, Deref, DerefMut},
slice,
};
Expand Down Expand Up @@ -1037,7 +1037,7 @@ impl<T> Option<T> {
match self {
Some(val) => val,
// SAFETY: the safety contract must be upheld by the caller.
None => unsafe { hint::unreachable_unchecked() },
None => unsafe { mem::MaybeUninit::uninit().assume_init() },
Copy link
Member

Choose a reason for hiding this comment

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

is this really better? that's disappointing.. how about using offset_of and accessing the value with directly with a pointer offset, entirely removing the condition?

Copy link
Author

Choose a reason for hiding this comment

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

I don't know if it's better in general - I just know it's better in at least one case. I think it's worth running a benchmark for it at least though. (which I can't do 🥺)

TIL about offset_of. uninit seems to be chucked pretty readily, but offset_of might be better in low opt level builds etc.

Copy link
Member

Choose a reason for hiding this comment

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

@Nilstrieb It doesn't seem to have the desired effect:
https://godbolt.org/z/z5fv4as4s

@alion02 Please make your new libcore compile.

}
}

Expand Down
6 changes: 3 additions & 3 deletions library/core/src/result.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@

use crate::iter::{self, FusedIterator, TrustedLen};
use crate::ops::{self, ControlFlow, Deref, DerefMut};
use crate::{convert, fmt, hint};
use crate::{convert, fmt, mem};

/// `Result` is a type that represents either success ([`Ok`]) or failure ([`Err`]).
///
Expand Down Expand Up @@ -1460,7 +1460,7 @@ impl<T, E> Result<T, E> {
match self {
Ok(t) => t,
// SAFETY: the safety contract must be upheld by the caller.
Err(_) => unsafe { hint::unreachable_unchecked() },
Err(_) => unsafe { mem::MaybeUninit::uninit().assume_init() },
}
}

Expand Down Expand Up @@ -1491,7 +1491,7 @@ impl<T, E> Result<T, E> {
debug_assert!(self.is_err());
match self {
// SAFETY: the safety contract must be upheld by the caller.
Ok(_) => unsafe { hint::unreachable_unchecked() },
Ok(_) => unsafe { mem::MaybeUninit::uninit().assume_init() },
Err(e) => e,
}
}
Expand Down
Loading