-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Add unstable core::mem::variant_count
intrinsic
#73418
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -830,6 +830,7 @@ symbols! { | |
v1, | ||
val, | ||
var, | ||
variant_count, | ||
vec, | ||
Vec, | ||
version, | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
// run-pass | ||
#![allow(dead_code)] | ||
#![feature(variant_count)] | ||
#![feature(never_type)] | ||
|
||
use std::mem::variant_count; | ||
|
||
enum Void {} | ||
|
||
enum Foo { | ||
A, | ||
B, | ||
C, | ||
} | ||
|
||
enum Bar { | ||
A, | ||
B, | ||
C, | ||
D(usize), | ||
E { field_1: usize, field_2: Foo }, | ||
} | ||
|
||
struct Baz { | ||
a: u32, | ||
b: *const u8, | ||
} | ||
|
||
const TEST_VOID: usize = variant_count::<Void>(); | ||
const TEST_FOO: usize = variant_count::<Foo>(); | ||
const TEST_BAR: usize = variant_count::<Bar>(); | ||
|
||
const NO_ICE_STRUCT: usize = variant_count::<Baz>(); | ||
const NO_ICE_BOOL: usize = variant_count::<bool>(); | ||
const NO_ICE_PRIM: usize = variant_count::<*const u8>(); | ||
|
||
fn main() { | ||
assert_eq!(TEST_VOID, 0); | ||
assert_eq!(TEST_FOO, 3); | ||
assert_eq!(TEST_BAR, 5); | ||
assert_eq!(variant_count::<Void>(), 0); | ||
assert_eq!(variant_count::<Foo>(), 3); | ||
assert_eq!(variant_count::<Bar>(), 5); | ||
assert_eq!(variant_count::<Option<char>>(), 2); | ||
assert_eq!(variant_count::<Option<!>>(), 2); | ||
assert_eq!(variant_count::<Result<!, !>>(), 2); | ||
} |
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.
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.
Returning a
usize
is incompatible with#![feature(repr128)]
.We don't currently expose a way for users to determine the discriminant type of an enum (see #70705). I think we have a few options here:
u128
. Assuming we never addu256/i256
to the language (both of which seem insane as an enum discriminant type), this would be compatible with enums with any number of variants.DiscriminantKind
trait, and have this function return a value of type<T as DiscriminantKind>::Discrimininant>
. This does not solve the problem of converting the value to an integer, but it avoids returning au128
for enums which don't need it.usize::MAX
variants are unsupported (#[repr(u128)]
enums with fewer variants are fine). We might want to consider changing this tou64
, though I don't actually see any use-case for having an enum with more than 4 billion variants (let alone on a 32-bit platform).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.
On second thought, Rust does have tier-3 support for a 16-bit platform (AVR). While having
u16::MAX
variants seems ridiculous, it seems slightly more questionable to forbid that compared tou32::MAX
(though again, I have no idea why anyone would do such a thing). Perhaps au32
is the way to go 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.
Seeing as the compiler uses an
IdxVec
to track declared variants we’re guaranteed to ICE if we have an enum with a number of variants exceedingusize::MAX
anyway so I think they’re implicitly unsupported.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.
Making it explicit is no bad thing though!
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.
Ah - I completely forgot about cross compilation...
u64
seems like the best bet thenThere 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.
Ok... Thinking about this some more, I think we stick with
usize
and add a note to say that if your enum has more thanusize::MAX
variants on the target platform, the return value is unspecified. The probability this ever happens is minuscule and we've covered our backs with the note.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.
The compiler could be running on a 64bit system but generate code for a 16bit system. So you cannot really deduce anything from the type the compiler is using 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.
I’m in favour of
<T as DiscriminantKind>::Discrimininant>
oru128
oru64
, in that specific order. Having safe and stable APIs that may return unspecified or incorrect results without a very very convincing reason is questionable.This is an implementation detail that can be changed in the future to adapt to changing needs. The return type of a stable API will get set in stone for however long Rust exists.
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.
Unfortunately, you can’t use
<T as DiscriminantKind>::Discriminant
because if I have an enum with256
variants the discriminant type will beu8
which can’t represent256
.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 should have struck this out because it doesn’t apply anyway in the case of cross comp