-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
RFC: Add Enums module for enum support through the @enum
macro
#10168
Conversation
@enum
macro
|
||
@test_throws MethodError convert(Base.Enums.Enum, 1.0) | ||
|
||
Base.Enums.@enum Fruit apple orange kiwi |
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.
Wouldn't it make more sense to import the @enums
macro and then use it bare? I'd expect that we export this from Base if this gets merged.
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 actually is exported from Base in this PR, I just tend to write tests as fully qualified as possible :)
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 fair, although I might argue that the fact that it's exported is something one might want to test for. After all, if it somehow was suddenly deleted from Base's exports that would certainly break a lot of code.
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 a good point.
I like this approach, and I think this is fine as-is. I agree scoping is the most significant remaining design issue. However I think we can live with this, as you can already wrap an enum in a module if you want the values hidden better. Should we automatically pick the smallest integer type that works for the given values? E.g. an enum with 3 values, and no specified integer values, could use |
Yeah, I originally thought of picking the smallest value, but went this this approach instead (using |
Yes, that approach is good. But in addition to that, we could pick |
That's true. I'll work on putting that in plus a few other tweaks. |
@test ff.n === 0xff | ||
@test overflowed.n === 0x00 | ||
|
||
@test_throws MethodError eval(macroexpand(:(Base.Enums.@enum Test1 _zerofp=0.0))) |
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.
Am I missing a better way to test error-throwing macros? This felt a little weird.
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.
This is pretty much it, but just eval
should be sufficient. It does macro expansion.
Alright, changed enum field name to |
…to fit all values
The CI tests all passed except a timeout on OSX (which I've tested locally). Any other comments/feedback? Responses to the questions I mentioned above? |
Should we be concerned about C storage compatibility as a default? Is there much call for passing arrays of enums? I can't recall many C APIs that do that, so I'm thinking no. |
I believe the enum type is up to the compiler, the C spec only requires that the enum type be big enough to hold all elements. |
To be sure, C will let you set flags to choose a different size, but you're generally safe assuming int. Possibly not for arrays, but it's common enough for structs. |
Any other comments? Good to merge? |
Conflicts: base/exports.jl test/runtests.jl
Rebased and added docs/NEWS entry. I'll merge this weekend unless there are other objections. |
Hmmmm, linux travis failed trying to download Rmath and OSX travis failed with the error below, which I believe is unrelated: ERROR: LoadError: MethodError: `<<` has no method matching <<(::Function, ::Function)
Closest candidates are:
<<(::Any, !Matched::Int32)
<<(::Any, !Matched::Integer)
in include at /private/tmp/julia/lib/julia/sys.dylib
in include_from_node1 at loading.jl:128
in process_options at /private/tmp/julia/lib/julia/sys.dylib
in _start at /private/tmp/julia/lib/julia/sys.dylib
while loading /private/tmp/julia/share/julia/test/runtests.jl, in expression starting on line 1 |
Failed in the same way on both win32 and win64 on appveyor, so seems to be related. |
RFC: Add Enums module for enum support through the `@enum` macro
Overloading The functionality is great, but as you pointed out it is very easy to overwrite functions in |
You have already claimed the Enum name, no harm in exporting it. Makes it easier to write julia> @enum Foo bar baz
julia> isa(baz, Enum)
ERROR: UndefVarError: Enum not defined
julia> isa(baz, Base.Enums.Enum)
true On Sat, Feb 21, 2015 at 3:08 PM, Jacob Quinn notifications@github.com
|
+1 for renaming |
Sorry if it's an obvious question, but why does enum counting start at zero? I would have guessed it starts at one like with Arrays. To me the documentation also suggests it starts at one. |
C compatibility. On Sun, Feb 22, 2015 at 4:43 PM, Ken-B notifications@github.com wrote:
|
Is this the behavior we want? julia> @enum(Foo, bar=1, baz=1)
julia> bar
barbaz::Foo
julia> is(bar, baz)
true |
Hmmm....good question. It's the same behavior as C, but C is also much more "simple" in it's implementation of just mapping to integers and that being more transparent. It seems like it could possibly be useful to allow, but also a tad confusing when you do something like |
I suspect that we should initially err on the side of conservatism and consider this to be an error. Just because C allows it doesn't mean it's a great idea; let's wait and see if anyone actually needs this, and if so, we'll have a little more sense of what it should do. Concatenating the symbol names, for example, seems a bit weird. |
I needed it today (mapping a C enum), but I don't know if we want to retain that behavior. Agree about the concatenating symbols part. |
Well, what behavior would make sense for your application? |
I've seen it used in Gtk to make aliases (eg they might make one with DOUBLE, TWO, and 2 as a modifier for CLICK. Which one gets used then can be dependent on user choice: limitations of a language binding variable naming rules vs. length) |
For the C interop case I think allowing this behavior and having |
With immutables, I don't think that's possible (or sensible) – how would you distinguish |
Yeah, definitely feels uncomfortable messing with |
It's inherently impossible to make |
I'm not really advocating that as a solution, only I feel that would be the desired behavior. |
Ok, so since that's impossible, I think we just need to pick either the first or last symbol as the canonical one. Which makes more sense in your use case? |
My experience with C enums makes me think that the first one is probably the one we'd want to go with. |
Usually this crops up to retain backwards compatibility in the API, so picking the first one would make sense. |
Ok, so there we have it: allow multiple bindings for enum values, but the first one is the canonical one. |
What about |
Hmm. That's a little tough. It seems like all the alternative names should show up, even if more than one refer to the same value. Either that or only the canonical names should show up. |
I'm coming around to the fact that this should just be an error. I've never come across a situation where this is absolutely necessary. We are just complicating things for convenience. |
Even better! |
The most important difference is that Julia provides a two way mapping, but C is restricted to a one way mapping. Assigning multiple names for the same value is convenient in some cases, but isn't the same need served by having the user manually type @enum(Foo, bar=1)
const Baz == bar |
I think I'm ok going either way. If we allow duplicates in declaration, then I think |
Is there an established way to automatically export all the values of an enum in a module? |
This builds largely on and supersedes the work by @vtjnash in #2988 (minus the bitmask business), with some modifications in line with comments in #3080 and the python PEP for enums.
Basically the usage is:
The RFC status is because there are still a few design questions that may be worth discussing:
@enum Fruit apple=-1
); I couldn't really think of a reason to disallow them; I think in some languages they're disallowed however@enum Fruit apple=0xff orange
,orange
will have value0x00
); I don't think this is a big deal in practice, but while writing tests it came upEnums.Enum
(could be used in a module for groups of enums, for example); we could allow this through syntax like@enum YellowFruits=Fruit banana lemon
, but I'm not sure if that's pretty or useful enoughenum
and enum members are fields of that class; this provides a nice scoping for enum members that belong to the enum and don't leak out into the current module namespace. In this implementation, enum member values are justconst enum_value = EnumType(x)
, which isn't bad, except if you inadvertently do something like@enum MyImportantEnum zero=0 one=1
, which overwrites the bindings for the Base functionszero
andone
. One idea I had for this would be, in the@enum
macro call, to wrap the enum type definition and enum member assignments in a baremodule. With allow overloading of a.b field access syntax #1974, you could then access enum member values only by doing something likeFruit.apple
(which is python's syntax) and this would access the enum member value within the auto-generated baremodule.I'd appreciate any comments/feedback.