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
repr(tag = ...)
for type aliases #3659base: master
Are you sure you want to change the base?
repr(tag = ...)
for type aliases #3659Changes from 8 commits
9da95c4
2ce777c
4975077
3302274
d20d743
07c2d94
b123535
9b22aba
b71923a
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.
Since we intend for
repr(discriminant = u8)
to be equivalent torepr(u8)
, then I'd like to propose that we perhaps shouldn't userepr(discriminant = ...)
since it might complicate a future possibility for enums.That future possibility — alluded to in this RFC — is that we someday might want to expand the accepted set of discriminant types of enums (e.g., to include things implementing
StructuralEq
). And, if we had such functionality, it might also be nice to declare up front what the discriminant type is. A natural syntax for this could be#[repr(discriminant = ...)]
.However,
repr(discriminant = ...)
, as proposed by this RFC, does not only set the discriminant type of an enum, but also affects its memory layout. This is not desirable for all enums. If this RFC is adopted as-is, we'd need to find an alternative way to say "the discriminant type of this enum is X, but I don't care what the enum's in-memory representation is".Given this, I'd like to suggest that this RFC consider
repr(tag = ...)
. Doing so has three advantages:repr(discriminant = ...)
syntactically reserved for the aforementioned future possibility in which the user explicitly provides the type of the discriminantIf not
repr(tag = ...)
than perhaps something else.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'm confused how labeling this a tag makes it less confusing. It's a shorter word, but that's about it.
We've already established that we're changing the memory layout by using
repr
.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.
Considering that these are often called "tagged unions" in contexts where memory layout matters, I do think the name is clearer. It's also going to be consistent with other in-flight changes to Rust's documentation: rust-lang/reference#1454 (comment)
What are the advantages of
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.
They are often called this, but we also explicitly use the term discriminant in places where it has an API meaning. See: https://doc.rust-lang.org/nightly/std/mem/fn.discriminant.html
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.
In the lang meeting today, we decided on
tag
. We felt that the existingmem::discriminant
API was unfortunate, and unloved in enough other ways, that we didn't need to weigh that heavily as precedent. See also here.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.
Very fair. I think that it would be worth proposing to deprecate & rename that API if this is the route we're going, to ensure uniformity.
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.
(Also, while the decision on the rename is resolved, I'm going to unresolve this until I update the RFC to rename the attribute, just so it's easier to track. Plus, it would be helpful to clarify all the finer details on how they should be written into the RFC before this gets marked as resolved.)