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

should get_unchecked and get_unchecked_mut be using unreachable!() ? #40

Open
oconnor663 opened this issue Oct 7, 2017 · 4 comments
Open

Comments

@oconnor663
Copy link
Contributor

oconnor663 commented Oct 7, 2017

I think the bad branches in those functions are reachable, if the caller passes in a key that's been removed. Should those branches be ordinary panics? Or should those functions maybe return Option<&T> / Option<&mut T>, like the get and get_mut functions do? (Or is it possible these functions don't have any callers in the wild, and we can remove them?)

@carllerche
Copy link
Member

If they are reachable, it should probably be changed to a panic instead. That said, unreachable is just a panic.

The functions are here to avoid any extra bounds checking.

@oconnor663
Copy link
Contributor Author

oconnor663 commented Oct 18, 2017

I wonder if there's a way to be even less safe, by unwrapping the Option without a branch. Maybe we could construct a mem::uninitialized instance of Some(T) (or rather...a Some(mem::uninitialized())?), figure out the pointer offset for the T inside, and mem::forget the "temporary" instance, and the compiler would just optimize all that code away to a constant? I need to learn to read assembly...

@oconnor663
Copy link
Contributor Author

Relevant: rust-lang/rfcs#1863

@tormol
Copy link
Contributor

tormol commented Mar 1, 2019

Making them less safe can now be done by using std::hint::unreachable_unchecked(), but that requires Rust 1.27.

Currently the compiler isn't inserting any trap (ud2) instructions either.

EDIT: I have a commit implementing this change, but I'm not opening a PR because I don't think this is worth making a breaking change for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants