-
Notifications
You must be signed in to change notification settings - Fork 13k
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 num_variants intrinsic for enums and safe wrapper in std::reflect. #13860
Conversation
This is useful for avoiding the classic C workaround: enum Shape {
Triangle = 0u,
Square,
Circle,
NUM_SHAPES
} |
This should really be a macro so it can be used as a constant, but the necessary information isn't available from the macro expander, it looks like. Open to suggestions. |
This sort of thing is really only useful if you can then iterate the range Maybe a better solution would be an intrinsic that evaluates to |
Intrinsic? Why not a derived trait (or anonymous |
Good point. Derived trait makes more sense. |
That's not true; I could use this in Servo today for things like creating a vector holding the buckets for different profiling categories, along with several other similar use cases. |
Yeah, so my use case is actually something like this: enum NS {
StructNS,
ValNS,
TypeNS,
ModNS,
}
struct Subscope {
table: SmallIntMap<NS, DefId>,
}
impl Subscope {
fn new() -> Subscope {
Subscope { table: SmallIntMap::with_capacity(num_variants::<NS>()) }
}
} That SmallIntMap could easily be a vector. It's more useful as a constant than as a slice, and I think a derived trait is far too much for use cases like that, when all you need to know is how many different distinct values you're going to need. |
A derived trait is much cleaner than a specialized intrinsic. And being able to derive let ns: NS = Bounded::max_value();
Subscope { table: SmallIntMap::with_capacity(ns as uint) } |
Bounded certainly is no good here -- presumably max_value() gives back 3, while I really want 4, and min_value() is totally useless here. Do you mean deriving a trait manually or the compiler doing it automatically? If the former, that's exactly what I'm trying to avoid. If the latter, the compiler needs to inject the extra impl next to the enum in the compilation unit, which hardly sounds cleaner. |
@kemurphy Good point about off-by-one, but you can trivially add 1 to that. And |
@kemurphy Here's what the code would look like with that: #[deriving(Bounded)]
enum NS {
StructNS = 3, // because maybe it's C compat?
ValNS,
TypeNS,
ModNS,
}
struct Subscope {
table: SmallIntMap<NS, DefId>,
}
impl Subscope {
fn new() -> Subscope {
let (min, max): (NS, NS) = (Bounded::min_value(), Bounded::max_value());
let count = max as uint - min as uint + 1;
Subscope { table: SmallIntMap::with_capacity(count) }
}
} Note that your proposed intrinsic won't handle this case, because the enum now starts at |
A deriving would be fine, but at the time those are expanded there's no way to associated an enum's type name with its variants. Bounded is still not the answer, in any case. let (min, max): (NS, NS) = (Bounded::min_value(), Bounded::max_value());
let count = max as uint - min as uint + 1; That is the most hilariously unfriendly snippet given the task it's trying to accomplish.
Uh, what? num_variants doesn't care about the values, it just returns the number of variants. Read the patch, please. |
What do you mean? Deriving knows what the variants of the enum it's deriving for are. And even if it can't ask for the actual discriminant values, all it needs is the AST to figure out which variant is smallest or largest.
How is that unfriendly? That's precisely how you'd handle the same enum in C. The classic C solution doesn't need this because the classic C solution has the enums start at 0, but that's not always the case. I've certainly seen C code that looks something like enum {
SECTION_A_START = 0x100,
A1 = SECTION_A_START,
A2,
A3,
SECTION_A_END = A3,
SECTION_B_START = 0x200,
B1 = SECTION_B_START,
B2,
B3
SECTION_B_END = B3
// ...
} and it needs the same solution to count the number of elements in a given section. Basically, your |
I am familiar with the C idiom.
You just answered your own question ;)
Again, that's false. I don't care about the values or even the range of values in the enum; I care about how many distinct values there are. Please, please, read the patch -- the intrinsic returns ty::enum_variants(ccx, def_id).len(). It's completely value-agnostic. |
Besides |
Countable would be just fine but it's impossible to implement currently (as is Bounded) -- derivings happen at syntax expansion time, so there's no AST map available to introspect which variants belong to a given enum. |
I have read the patch. What you seem to be ignoring is the fact that you're trying to add an intrinsic for a niche use-case. That's a bad idea, and it's made worse by the fact that there is a better generalized solution that can be implemented that covers a lot more use cases, that works just fine for you. But you're rejecting it because you consider it to make your code slightly uglier. |
I would prefer the generalized solution if it were possible to implement. This is hardly a niche use case though -- preallocating the necessary size of a map or vector is fairly common practice in the systems world. Refer to @jdm's comment, for example. |
(That is to say, I would prefer Countable for this particular problem.) |
What do you mean? Of course Deriving knows what variants your enum has. It can't possibly work otherwise! What exactly do you think impl Eq for Foo {
fn eq(&self, other: &Foo) -> bool {
intrinsics::eq(self, other)
}
} It's evaluating to impl Eq for Foo {
fn eq(&self, other: &Foo) -> bool {
match self {
Foo1 => match other {
Foo1 => true,
_ => false
},
Foo2 => match other {
Foo2 => true,
_ => false
}
// ...
}
}
} This very clearly knows what the variants are. |
It looked like derivings happen at the same phase as macro expansion, and it's not available there...? Is that really what Eq expands to? I guess that would make sense since at that point we have a notion of meta-items and items and how they're attached. If so then I guess it is possible there. Countable being a super-trait of Bounded though is still wrong. enum Thing {
Foo = 42,
Bar = 1337,
Quack = 116,
} And I'm fairly sure 1337 - 42 isn't 3. ;) |
Only if pub trait Countable : Bounded {
fn count() -> uint;
} There could be a default implementation like pub trait Countable : Bounded {
fn count() -> uint {
let min: Self = Bounded::min_value();
let max: Self = Bounded::max_value();
max - min + 1
}
} which would then be reimplemented for the types that actually do have holes, but I don't know if that's actually a good idea. It would mean that, for a type without holes, you can implement it using The only real issue with |
I disagree that Countable would be undefined for integer types. Countable would count the range of representable values -- for u64 and i64 both, there are u64::max_value() possible values. The advantage to num_variants is it works on any enum without having to opt into it, but it is totally usable to just have |
I will work on Countable as an alternative to this since apparently the knee-jerk reaction to an intrinsic is that it's dirty and a bad idea. (Not to say that an intrinsic is the best solution here, but it's exactly the kind of thing an intrinsic should do.) |
Nope, there are |
Ah, indeed. Good point. |
@kemurphy Also, as a thought exercise, what would the impl of |
Presumably f32 wouldn't implement Countable. It's probably not particularly useful for the integer types to implement Countable, either, which would indeed make it weird since it seems like no other libstd types are likely to derive it.... Which brings us back to the intrinsic. |
I think the most reasonable way to think about |
@alexcrichton: ping? Who else should be in on this discussion? If this isn't desired can you or one of the other core devs suggest an approach that is not "implement |
@kemurphy What is your opposition to "implement let max: MyEnum = Bounded::max_value();
let min: MyEnum = Bounded::min_value();
let count = max as uint - min as uint + 1; is not a workable solution? I expect that implementing As a side note, if we do implement |
@kballard: My opposition is that this is not a bounds problem, it is simply a number of variants problem. It doesn't typecheck, one might say. :) let max: MyEnum = Bounded::max_value();
let min: MyEnum = Bounded::min_value();
let count = max as uint - min as uint + 1; While this works for my particular use case, in my opinion it is very unwieldy and overly complex. Deriving Bounded also does not work for C-like enums with holes in them, for 2 reasons:
Also, generally, deriving Bounded for an enum to me would imply that you could take any old |
To elaborate on this, at the very least the expression I'm looking for would be a single function call that doesn't have to do any math (regardless of whether or not it gets constant-folded away anyway). Something like |
@kemurphy In a dense C-like enum, calculating the number of variants is identical to calculating the bounds. Regarding C-like enums with holes:
True. But is this a use-case that matters right now?
I disagree. Consider the type For reference, I think I guess to make it clearer, the concept of a hole in a type only exists if you also have the concept of addition with that type. You can then define holes as addition operations that result in illegal values (either wrapping or saturating on overflow, to avoid that being considered a hole). But of course addition isn't defined for C-like enums (by default). |
@kemurphy Basically what I'm trying to say is, I think Granted, if you can get buy-in to do |
Agreed.
No, but ideally the solution here would be identical to and work for such enums. If it solves my use case, it should solve all other use cases too. I'd like to preserve at least a modicum of generality across all C-like enums. :)
This explanation is heavily dependent on that being the way you define Bounded, but I agree on all fronts.
I also agree, but will add that I don't think I should be required to derive Bounded in order to derive Countable, when I don't care about the bounds (and again we get back to how that solution still doesn't work for enums with holes). |
The need for calculating the number of variants of a C-like enum that has holes is much less obvious than the need for calculating the number of variants of a dense enum. A C-like enum that has holes has them because the discriminant has been explicitly set for various variants, and such an enum seems unlikely to be used in a scenario where the variant count alone is enough information (which, honestly, seems really only useful for setting the capacity of a key-value map). For comparison's sake, in C, needing the number of variants is generally used because the code wants to calculate all the variant values.
Any type that is Here's another suggestion. A derivable trait that provides a method that returns a |
While a fair point, and utility aside, it still feels wrong to me to provide a "get the number of variants" solution that a) depends on the
I should clarify -- I agree with the Bounded bound on Countable; I would like to see a
This is something I can get behind. What would the name of such a trait be? |
Another alternative may be a separate attribute entirely like |
Something like that also means we don't have to add an unused trait to libstd. |
Associated values is the real solution here, so you can just say Until such time as we get associated values, I think trying to emit a static is a bad API. So right now, I think the best solution is the trait that returns pub trait EnumValues {
fn enum_values() -> &'static [Self];
} Although since this only works for C-like enums, it would be nice to put |
Yeah, associated values is the real answer here. For the name of the trait, what about |
pub trait Discrete {
pub fn domain() -> &'static [Self]; // or pub fn values()
} |
Why |
Hm, yeah Discrete probably isn't the right word I was looking for something like Finite, I guess? With Discrete I was thinking like "discrete set of values"; the integral types also apply but in that case the trait would look more like pub trait Discrete {
pub fn values<T:Iterator>() -> T<Self>;
} |
Enumerable, maybe? |
pub trait Enumerable {
fn values() -> &'static Self;
fn iter<T: Iterator>() -> T<Self>;
} |
That pub trait Enumerable<T: Iterator<Self>> {
fn values() -> &'static Self;
fn iter() -> T;
} and then the implementation would have to provide the concrete iterator type. I don't think this is worthwhile. The same iterator can be accomplished just by calling |
Oh, derp, I knew that. Oops. Yeah only values() is necessary. |
The more that this progresses into "let's add some more deriving modes" territory, the more this is progressing into "this needs an RFC" territory. There are many ideas being discussed here, and I don't want to have something slide in without general agreement. Is the opinion so far that the intrinsic may not be the best route to go? |
@alexcrichton: My opinion is that for things like my use case, the deriving is more tasteful and probably the preferable solution. That being said, I think the intrinsic is still useful from a reflection perspective, but I'm okay with letting it go in favor of the trait. |
I can throw together a PR for the trait/deriving, and if comments seem to indicate that an RFC is necessary I can write one up. Does that work? |
The scope of this new deriving mode sounds large enough to require going through the RFC process first, but you're welcome to sketch it out and see how it goes! |
Sure thing. As for this one, r? for reflection or should I just close it? |
It sounds like the opinion is that the deriving-based solution should be attempted first to see how it compares to the intrinsic-based solution. If it's ok with you as well, I'm going to close this for now, but feel free to reopen if the deriving-based solution doesn't work out! |
Works for me, thanks! |
fix a bunch of clippy lints fixes a bunch of clippy lints for fun and profit i'm aware of this repo's position on clippy. The changes are split into separate commits so they can be reviewed separately
This adds an intrinsic to query the number of variants in an enum, along with a safe wrapper for it in std::reflect:
If the type parameter is not an enum, the result is 0.