-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 Enum.size #5743
Add Enum.size #5743
Conversation
I'd use |
I was thinking about that, and finished with: Enum is not a container, it doesn't really have a size. So |
However |
If it's anything it's |
Ok, I'll change to My usecase is that I have a huge enum (~400+ members) that are used as a named index in an Array (or another container), and I want to create the Array with at least the minimal size (the numbers of enum members), to make sure it won't need to realloc or anything. So basically I want to do |
Weird, the build failed with |
@bew , it'll be clearer to rebase to one commit e.g |
src/enum.cr
Outdated
# ``` | ||
def self.size : Int32 | ||
{% if @type.has_attribute?("Flags") %} | ||
{{ @type.constants.size - 2 }} |
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.
Actually this should be the popcount of all the constants &
ed together.
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 what? what does popcount
mean?
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.
Number of non-zero bits. That's significant if there is a flag that sets multiple bits.
I'm not sure if that's the right behaviour, but generally this method is probably not that much relevant for Flag enums anyway.
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.
For enums it should be the number of unique values the enum has (currently this is not unique), for flags it should be the number of actual flag bits covered by all the options. For example in some enums you have A = 0b01
, B = 0b10
and Both = 0b11
, That's 3 flag constants but only 2 flag bits, which popcount
solves.
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 question is, should .size
return the number of constants or the number of unique values?
Quoting @straight-shoota on this comment:
enum A
A = 1
B = 2
C = 1
end
pp A::A # => A
pp A::C # => A https://carc.in/#/r/3scm Edit: seems like I'm wrong, it's just the printing that is 'broken' as it'll print the first enum member with that value, so the number of constant is still 3 in this case, not 2. |
I don't see a valid usecase where having the number of constant is more important than the number of unique values. |
Ok I made it work for enum flags with popcount, but I didn't find a way to find the number of unique normal enum values without an "ugly" array alloc' just to count the number of unique values.. Any ideas? |
@bew what is the status on this PR it looks like you might have some failing tests? |
@wontruefree I need #5903 in order to access the values of each enum fields at compile time, and get the number of unique enum values for And as it currently goes (see last Ary's comment on that other issue) it's too complex and useless... 😟 If you have a good usecase for this please go ahead |
Actually I managed to get the values of each enum fields at compile time using a combination of And the problem of So there might not be any issues left, 'just waiting for the builds to finish.. 👍 |
spec/std/enum_spec.cr
Outdated
end | ||
|
||
it "gives number of unique enum values" do | ||
SpecNonUniqueEnum.size.should eq 2 |
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.
IMO this should count number of enum members, not values.
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.
That's the big question of this PR I guess, but I can't find a usecase for this behavior..
Do you have one?
If the 2 behaviors are legit, maybe we can have 2 functions size
& uniq_size
or sth..
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.
There's no need for uniqness if we're talking about members.
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.
For me size
should be the number of members. If you want to know the number of uniq values, select all members and call uniq
.
Since it seems to be the preferred behavior, I changed back FWIW, to get uniq values, check the previous version: 9f58719 |
One of travis build fails for memory error (the fork one), otherwise it should be good to go? |
# ``` | ||
def self.size : Int32 | ||
{% if @type.has_attribute?("Flags") %} | ||
{{ @type }}::All.value.popcount |
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.
Do we still want to dedupe for flags, and not for non-flags enums?
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.
It seemed to be preferable to have Enum.size
return the number of members for non-flags, I guess I should change it for flags too? (the popcount can always easily be done by the user, or added later under another name)
Or even rename it to Enum.count_members
to avoid any mis-interpretation?
Hello, I'm cleaning up old PRs of mine on projects I don't follow anymore. What do we do with this PR ? Close? or someone wants to take it? |
If you don't plan to work on this, I'd suggest to close it. It's easy to open a new PR. Maybe we should add an issue to track this enhancement. |
Okay, closing it then |
I just stepped on the need to know the number of enum members in an enum, and was surprised there wasn't a class method
Enum.size
orEnum.count
, so here it is.