-
-
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
Disallow negative enum flag values #6159
Conversation
The failing test is: (introduced in 0eb47f5) it "casts All value to base type" do
run(%(
@[Flags]
enum Foo
A = 1 << 30
B = 1 << 31
end
Foo::All.value
)).to_i.should eq(-1073741824)
end I'm not sure how to understand it and make the proper changes if needed. It fails because I'm wondering now: should the default enum flag base type be UInt32 instead? |
Unsigned I agree ~ for flags. Signed doesn't make sense 👍 |
Any opinions from core devs? |
C# allows negative enums just fine. Is there a bug related to this, or it's just something that we are considering we could change? Because if there's no bug, and everything is working fine, I'd rather not touch the language, specially if there's a new failing spec because of this (I understand it gives a negative value, but bitwise for flags it makes sense, even if it overflows). |
@asterite does C# have flags enums? |
@RX14 Yes. That's where we got the inspiration from ;-) And flags enum just add |
It turns out negative enum flags are quite buggy: https://play.crystal-lang.org/#/r/4rdj |
@asterite (and core team) as showed in previous comment, flags enums with negative values are quite buggy from user-point-of-view, I'd suggest we make enum flags always unsigned, wdt? |
@bew the sample you show as buggy is not because the enums can be negative but because you can define overlapping values. It is not obvious what is the value assigned to the next member when overriding one value, that could be improved/reviewed maybe. The bug I am aware of is that if the enum base is changed to a 64 bits integer then the internal counter is not using all the available range of values, hence 64 bits integer enum needs to be explicitly defined. |
Indeed that's better said.. This PR can be closed then I guess
Nice find, didn't find an issue about this yet, will you open one? |
Seems this can be closed. |
Closes #5955