-
-
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
Non-32-bit enums / "Typed" enums #18826
Conversation
vals = Array{Tuple{Symbol,Integer}}(0) | ||
lo = hi = 0 | ||
i = Int32(-1) | ||
i = zero(basetype) |
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 switched this from incrementing before a symbol to incrementing afterward because that seemed to be the clearest way to support unsigned base types.
hasexpr = false | ||
for s in syms | ||
if isa(s,Symbol) | ||
if i == typemax(typeof(i)) | ||
if i == typemin(basetype) && !isempty(vals) |
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 must have wrapped if it's the typemin and there was a previous iteration.
Base.convert(::Type{Integer}, x::$(esc(typename))) = box($(basetype), x) | ||
Base.convert{T<:Integer}(::Type{T}, x::$(esc(typename))) = convert(T, box($(basetype), x)) | ||
Base.write(io::IO, x::$(esc(typename))) = write(io, $(basetype)(x)) | ||
Base.read(io::IO, ::Type{$(esc(typename))}) = $(esc(typename))(read(io, $(basetype))) |
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 didn't see a way to make these work without overloading them for each enum type. I toyed with making Enum
itself generic, but that broke everything in fun ways.
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'd like to hear more about that, as having Enum{T}
seems like a nice way to get this if possible.
@@ -130,9 +143,9 @@ macro enum(T,syms...) | |||
end | |||
end | |||
end | |||
if isa(T,Symbol) |
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.
By my reading of the code this can't ever be false.
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.
You're right; I think that's a vestige from when enums could have type parameters.
@@ -130,9 +143,9 @@ macro enum(T,syms...) | |||
end | |||
end | |||
end | |||
if isa(T,Symbol) | |||
if isa(typename,Symbol) |
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.
But I kept it here just in case I missed something.
@@ -81,59 +81,20 @@ end | |||
@test_throws ArgumentError eval(:(@enum Test22 1=2)) | |||
|
|||
# other Integer types of enum members | |||
# TODO - not yet supported |
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.
A lot of these .val
related tests don't really make sense with bitstype-based 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.
Some comments
let insts = ntuple(i->$(esc(typename))($values[i]), $(length(vals))) | ||
Base.instances(::Type{$(esc(typename))}) = insts | ||
end | ||
Base.names(::Type{$(esc(typename))}) = $(esc(names)) |
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 would not add this in this commit, as it's not really related AFAICT.
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.
Done
@@ -130,9 +143,9 @@ macro enum(T,syms...) | |||
end | |||
end | |||
end | |||
if isa(T,Symbol) |
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.
You're right; I think that's a vestige from when enums could have type parameters.
Base.convert(::Type{Integer}, x::$(esc(typename))) = box($(basetype), x) | ||
Base.convert{T<:Integer}(::Type{T}, x::$(esc(typename))) = convert(T, box($(basetype), x)) | ||
Base.write(io::IO, x::$(esc(typename))) = write(io, $(basetype)(x)) | ||
Base.read(io::IO, ::Type{$(esc(typename))}) = $(esc(typename))(read(io, $(basetype))) |
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'd like to hear more about that, as having Enum{T}
seems like a nice way to get this if possible.
Base.convert(::Type{Integer}, x::$(esc(typename))) = box($(basetype), x) | ||
Base.convert{T<:Integer}(::Type{T}, x::$(esc(typename))) = convert(T, box($(basetype), x)) | ||
Base.write(io::IO, x::$(esc(typename))) = write(io, $(basetype)(x)) | ||
Base.read(io::IO, ::Type{$(esc(typename))}) = $(esc(typename))(read(io, $(basetype))) |
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 earlier definitions of convert, read, and write in this file should be removed.
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 had another go at doing Enum{T}
and the problems I had last time didn't reappear. So that's nice.
I didn't see a way to implement Base.read
without a helper method to extract the type, which I think is a bit unfortunate. In principle I want to do
Base.read{EnumT<:Enum{T<:Integer}}(io::IO, ::Type{EnumT}) = EnumT(read(io, T))
but we don't have triangular dispatch yet. :)
I think this is a good idea: would you mind updating the docs? |
if hasexpr && values != unique(values) | ||
throw(ArgumentError("values for Enum $typename are not unique")) | ||
end | ||
blk = quote | ||
# enum definition | ||
Base.@__doc__(bitstype 32 $(esc(T)) <: Enum) | ||
Base.@__doc__(bitstype $(basetype.size * 8) $(esc(typename)) <: Enum{$(basetype)}) |
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 should probably be sizeof(basetype)
|
||
Base.convert{T<:Integer}(::Type{T}, x::Enum) = convert(T, box(Int32, x)) | ||
abstract Enum{T<:Integer} |
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 guess we need to think about whether or not it makes sense to have Enum
be a parametric type.
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 did it this way because Jeff said it'd be nice. I'm not attached either way, although it does seem elegant to me.
OTOH, if it is parametric, a nice but incompatible option would be to parameterize on bits length. I can imagine some codec package - e.g. for ASN - finding it convenient to refer to the type of enums that are N bits long.
Adds `@enum Foo::*IntegerType*` to specify to use a bitstype with the same size as the specified integer type. The default type remains `Int32`. Summary: * The type must be an integer bitstype. * `read` and `write` read/write the right number of bytes. * `convert(Integer, value)` converts into the type specified in the `@enum`. Converting to a specific integer types works the same way as before. * There's no accessor to determine the specified type, but `typeof(convert(Integer, value))` will do. * Some of the old typed enums tests have been updated, but many of them relate to the `val` field, which isn't really meaningful now. Tangentally related changes: * `names` is overloaded for enums, and returns the names of the enumerants. Not totally sure that is the right function for the concept, though.
@test typeof(_zero_Test7.val) <: UInt8 | ||
|
||
@test_throws ArgumentError eval(:(@enum Test8 _zero="zero")) | ||
@test_throws ArgumentError eval(:(@enum Test9 _zero='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.
why is it that these don't throw now?
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.
Oops. They still do. I deleted too many lines when removing the .val
tests.
|
||
Create an `Enum` type with name `EnumName` and enum member values of | ||
Create an [`Enum`]{`BaseType`} subtype with name `EnumName` and enum member values of |
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.
what are you going for with the formatting here?
@@ -42,33 +44,42 @@ f (generic function with 1 method) | |||
julia> f(apple) | |||
"I'm a FRUIT with value: 1" | |||
``` | |||
|
|||
`BaseType`, which defaults to `Int32`, must be a bitstype subtype of Integer. Member values can be converted between | |||
the enum type and `BaseType`. `read` and and `write` perform these conversions automatically. |
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.
"and and" repeated
Adds
@enum Foo::*IntegerType*
to specify to use a bitstype with the same size as the specified integer type. The default type remainsInt32
.Use case: I want to use an enum for a field in a structure passed to a C function, and I don't get to decide the field size.
Summary:
read
andwrite
read/write the right number of bytes.convert(Integer, value)
converts into the type specified in the@enum
. Converting to a specific integer types works the same way as before.typeof(convert(Integer, value))
will do.val
field, which isn't really meaningful now.Tangentally related changes:
names
is overloaded for enums, and returns the names of the enumerants. Not totally sure that is the right function for the concept, though.Not done (yet):
@enum
.Unanswered questions: