Skip to content

Implement PartialOrd and Ord for Discriminant #106418

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+27 −4
Open

Implement PartialOrd and Ord for Discriminant #106418

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3237c31
Implement `Ord` for `Discriminant`
EFanZh Jan 3, 2023
2fd6091
Update library/core/src/mem/mod.rs
EFanZh Nov 15, 2023
db4656f
Update library/core/src/mem/mod.rs
EFanZh Nov 15, 2023
2ff6f20
Improve stability documentation
daxpedda Nov 15, 2023
dbc3154
Fix feature name
EFanZh Nov 15, 2023
36ce983
Update library/core/src/mem/mod.rs
EFanZh Nov 15, 2023
ec64db7
Update library/core/src/mem/mod.rs
EFanZh Jan 26, 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
12 changes: 11 additions & 1 deletion library/core/src/marker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,17 @@ pub trait DiscriminantKind {
/// The type of the discriminant, which must satisfy the trait
/// bounds required by `mem::Discriminant`.
#[lang = "discriminant_type"]
type Discriminant: Clone + Copy + Debug + Eq + PartialEq + Hash + Send + Sync + Unpin;
type Discriminant: Clone
+ Copy
+ Debug
+ Eq
+ Hash
+ Ord
+ PartialEq
+ PartialOrd
+ Send
+ Sync
+ Unpin;
}

/// Compiler-internal trait used to determine whether a type contains
Expand Down
21 changes: 17 additions & 4 deletions library/core/src/mem/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1106,18 +1106,31 @@ impl<T> fmt::Debug for Discriminant<T> {
}
}

#[stable(feature = "discriminant_ord", since = "CURRENT_RUSTC_VERSION")]
impl<T> cmp::PartialOrd for Discriminant<T> {
fn partial_cmp(&self, other: &Self) -> Option<cmp::Ordering> {
self.0.partial_cmp(&other.0)
}
}

#[stable(feature = "discriminant_ord", since = "CURRENT_RUSTC_VERSION")]
impl<T> cmp::Ord for Discriminant<T> {
fn cmp(&self, other: &Self) -> cmp::Ordering {
self.0.cmp(&other.0)
}
}

/// Returns a value uniquely identifying the enum variant in `v`.
///
/// If `T` is not an enum, calling this function will not result in undefined behavior, but the
/// return value is unspecified.
///
/// # Stability
///
/// The discriminant of an enum variant may change if the enum definition changes. A discriminant
/// of some variant will not change between compilations with the same compiler. See the [Reference]
/// for more information.
/// `Discriminant` is an opaque wrapper around the enum discriminant, therefore it's value will
/// change when the enum definition changes. See the [Reference] for more information.
///
/// [Reference]: ../../reference/items/enumerations.html#custom-discriminant-values-for-fieldless-enumerations
/// [Reference]: ../../reference/items/enumerations.html#discriminants
Copy link
Contributor

@daxpedda daxpedda Nov 15, 2023

Choose a reason for hiding this comment

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

I don't actually understand why the original documentation was worded like this. It might be that back then discriminant were not well defined, but going by the Rust Reference, they are now. #36823 is the original source of the wording, but I couldn't find anything specific I'm looking for.

It should also have nothing to do with the compiler version, because even though it is true that the compiler can apply optimization to even completely remove the discriminant from the enum, that is not what discriminant_value() returns, it returns the actual discriminant as it is defined by the reference.

I copied most of the wording from the reference here. The wording could expand to explicitly say that the output is unstable, but I felt this is already much clearer now.

@Amanieu let me know what you think.

Copy link
Member

@RalfJung RalfJung Nov 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It should also have nothing to do with the compiler version, because even though it is true that the compiler can apply optimization to even completely remove the discriminant from the enum, that is not what discriminant_value() returns, it returns the actual discriminant as it is defined by the reference.

Terminology note: in the compiler we've (mostly) transitioned to call the thing that is actually encoded in memory the "tag". So, there's a discriminant, whose value is defined by the reference, and then the discriminant is stored in memory in the "tag" by some unspecified means. There might be no tag at all (e.g. for Option<!>), or the tag could be in a niche, or it could be exactly the discriminant, or it could be some offset version of the discriminant -- whatever, the relevant part is that SetDiscriminant converts the discriminant to a tag that is being stored, and GetDescriminant decodes the tag to recover the discriminant.

Copy link
Contributor

@daxpedda daxpedda Nov 15, 2023

Choose a reason for hiding this comment

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

I've never heard of GetDiscriminant, is that what discriminant_value() uses?

Copy link
Member

@RalfJung RalfJung Nov 15, 2023

Choose a reason for hiding this comment

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

These are the internal MIR operations for getting/setting the discriminant. And yes the underlying implementation of GetDiscriminant is shared with the discriminant_value intrinsic.

///
/// # Examples
///
Expand Down