Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement
PartialOrd
andOrd
forDiscriminant
#106418base: master
Are you sure you want to change the base?
Implement
PartialOrd
andOrd
forDiscriminant
#106418Changes from 5 commits
3237c31
2fd6091
db4656f
2ff6f20
dbc3154
36ce983
ec64db7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
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 whatdiscriminant_value()
uses?There was a problem hiding this comment.
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.