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

Swap the 2 variants of Option<T> #49499

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
111 changes: 106 additions & 5 deletions src/libcore/option.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,23 +146,23 @@
#![stable(feature = "rust1", since = "1.0.0")]

use iter::{FromIterator, FusedIterator, TrustedLen};
use {mem, ops};
use {cmp, intrinsics, mem, ops};

// Note that this is not a lang item per se, but it has a hidden dependency on
// `Iterator`, which is one. The compiler assumes that the `next` method of
// `Iterator` is an enumeration with one type parameter and two variants,
// which basically means it must be `Option`.

/// The `Option` type. See [the module level documentation](index.html) for more.
#[derive(Clone, Copy, PartialEq, PartialOrd, Eq, Ord, Debug, Hash)]
#[derive(Clone, Copy, PartialEq, Eq, Debug, Hash)] // PartialOrd and Ord by hand below.
#[stable(feature = "rust1", since = "1.0.0")]
pub enum Option<T> {
/// No value
#[stable(feature = "rust1", since = "1.0.0")]
None,
/// Some value `T`
#[stable(feature = "rust1", since = "1.0.0")]
Some(#[stable(feature = "rust1", since = "1.0.0")] T),
/// No value
Copy link
Contributor

Choose a reason for hiding this comment

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

Document (in a comment) the reason for this order and the manual impls below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should it a doc comment, or just a comment in the source?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some comments and made them reference each other.

#[stable(feature = "rust1", since = "1.0.0")]
None,
}

/////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -981,6 +981,107 @@ impl<T> From<T> for Option<T> {
}
}

// The Option<T> type used to be defined as { None, Some(T) }, but for codegen
// reasons we reversed it in #49499 to reduce the amount of work that needs
// to be done for Result<T, ()> <-> Option<T> conversions. Keeping the derived
// Ord and PartialOrd implementations would make that swap a breaking change.
#[stable(feature = "rust1", since = "1.0.0")]
impl<T> Ord for Option<T>
where
T: Ord,
{
#[inline]
fn cmp(&self, other: &Self) -> cmp::Ordering {
let self_discr = unsafe { intrinsics::discriminant_value(self) } as isize;
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't this use is_none() or is_some()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I guess I could use is_none, I'll check that it still produces the same code as for
Result<T, ()>.

let other_discr = unsafe { intrinsics::discriminant_value(other) } as isize;
if self_discr == other_discr {
match (self, other) {
(&Some(ref this), &Some(ref other)) => this.cmp(other),
_ => cmp::Ordering::Equal,
}
} else {
other_discr.cmp(&self_discr)
}
}
}

// See comment on the Ord impl above.
#[stable(feature = "rust1", since = "1.0.0")]
impl<T> PartialOrd for Option<T>
where
T: PartialOrd,
{
#[inline]
fn partial_cmp(&self, other: &Self) -> Option<cmp::Ordering> {
let self_discr = unsafe { intrinsics::discriminant_value(self) } as isize;
let other_discr = unsafe { intrinsics::discriminant_value(other) } as isize;
if self_discr == other_discr {
match (self, other) {
(&Some(ref this), &Some(ref other)) => this.partial_cmp(other),
_ => Some(cmp::Ordering::Equal),
}
} else {
other_discr.partial_cmp(&self_discr)
}
}

#[inline]
fn lt(&self, other: &Self) -> bool {
let self_discr = unsafe { intrinsics::discriminant_value(self) } as isize;
let other_discr = unsafe { intrinsics::discriminant_value(other) } as isize;
if self_discr == other_discr {
match (self, other) {
(&Some(ref this), &Some(ref other)) => this < other,
_ => false,
}
} else {
other_discr < self_discr
}
}

#[inline]
fn le(&self, other: &Self) -> bool {
let self_discr = unsafe { intrinsics::discriminant_value(self) } as isize;
let other_discr = unsafe { intrinsics::discriminant_value(other) } as isize;
if self_discr == other_discr {
match (self, other) {
(&Some(ref this), &Some(ref other)) => this <= other,
_ => true,
}
} else {
other_discr <= self_discr
}
}

#[inline]
fn gt(&self, other: &Self) -> bool {
let self_discr = unsafe { intrinsics::discriminant_value(self) } as isize;
let other_discr = unsafe { intrinsics::discriminant_value(other) } as isize;
if self_discr == other_discr {
match (self, other) {
(&Some(ref this), &Some(ref other)) => this > other,
_ => false,
}
} else {
other_discr > self_discr
}
}

#[inline]
fn ge(&self, other: &Self) -> bool {
let self_discr = unsafe { intrinsics::discriminant_value(self) } as isize;
let other_discr = unsafe { intrinsics::discriminant_value(other) } as isize;
if self_discr == other_discr {
match (self, other) {
(&Some(ref this), &Some(ref other)) => this >= other,
_ => true,
}
} else {
other_discr >= self_discr
}
}
}

/////////////////////////////////////////////////////////////////////////////
// The Option Iterators
/////////////////////////////////////////////////////////////////////////////
Expand Down
2 changes: 1 addition & 1 deletion src/test/compile-fail/issue-2111.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

fn foo(a: Option<usize>, b: Option<usize>) {
match (a,b) {
//~^ ERROR: non-exhaustive patterns: `(None, None)` not covered
//~^ ERROR: non-exhaustive patterns: `(Some(_), Some(_))` not covered
(Some(a), Some(b)) if a == b => { }
(Some(_), None) |
(None, Some(_)) => { }
Expand Down
6 changes: 3 additions & 3 deletions src/test/mir-opt/match_false_edges.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ fn main() {
// _2 = std::option::Option<i32>::Some(const 42i32,);
// _3 = discriminant(_2);
// _6 = discriminant(_2);
// switchInt(move _6) -> [0isize: bb6, 1isize: bb4, otherwise: bb8];
// switchInt(move _6) -> [0isize: bb4, 1isize: bb6, otherwise: bb8];
// }
// bb1: {
// resume;
Expand Down Expand Up @@ -119,7 +119,7 @@ fn main() {
// _2 = std::option::Option<i32>::Some(const 42i32,);
// _3 = discriminant(_2);
// _6 = discriminant(_2);
// switchInt(move _6) -> [0isize: bb5, 1isize: bb4, otherwise: bb8];
// switchInt(move _6) -> [0isize: bb4, 1isize: bb5, otherwise: bb8];
// }
// bb1: {
// resume;
Expand Down Expand Up @@ -183,7 +183,7 @@ fn main() {
// _2 = std::option::Option<i32>::Some(const 1i32,);
// _3 = discriminant(_2);
// _8 = discriminant(_2);
// switchInt(move _8) -> [1isize: bb4, otherwise: bb5];
// switchInt(move _8) -> [0isize: bb4, otherwise: bb5];
// }
// bb1: {
// resume;
Expand Down