Skip to content
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

Different problems with @enum #15728

Open
SimonDanisch opened this issue Apr 1, 2016 · 6 comments
Open

Different problems with @enum #15728

SimonDanisch opened this issue Apr 1, 2016 · 6 comments
Labels
performance Must go faster

Comments

@SimonDanisch
Copy link
Contributor

I found a few problems with the current enum implementation:

  • if you have very small and very large values, membershiptest will be slow or even throw OutOfMemory exceptions.
  • c enums allow fields with the same values, so @enum can't be use with Clang to wrap c.
  • in c you very often use enum1 & enum2 == enumx, this functionality might be wanted in Base as well
  • you could move out more functions to the abstract type, when defining enum_values & enum_names, which return the sorted list of enums (i initially thought the number of emitted functions are the reason that compilation is so slow)

Last point got a bit less interesting after fixing the other points made everything compile fast already.
I still wrote a toy implementation of it:
https://gist.github.com/SimonDanisch/88f73018d6bf5219574ec1c679bc76ef

julia> @time run(`julia "using Vulkan"`) # Base.@enum
  5.362600 seconds (100.65 k allocations: 4.504 MB)

julia> @time run(`julia "using Vulkan"`) # CEnum.@cenum
  5.220823 seconds (60 allocations: 2.266 KB)

What we should at least use from this is:

function is_unit_range(array)
    isempty(array) && return true # well in practice extrema will rather fail, but there is 0:0
    lastval = first(array)
    for val in rest(array, 2)
        val-lastval == 1 || return false
    end
    return true
end
# generate code to test whether expr is in the given set of values
function membershiptest(expr, values)
    lo, hi = extrema(values)
    sv = sort(values)
    if is_unit_range(sv)
        :($lo <= $expr <= $hi)
    elseif length(values) < 20
        foldl((x1,x2)->:($x1 || ($expr == $x2)), :($expr == $(values[1])), values[2:end])
    else
        :($expr in $(Set(values)))
    end
end

For the unique values, we should decide if there should be an extra type Cenum, or if we just don't guarantee mappings to correct names if you decide to use multiple fields with the same value.

Best,
Simon

@SimonDanisch
Copy link
Contributor Author

@vchuravy

@yuyichao
Copy link
Contributor

yuyichao commented Apr 1, 2016

if you have very small and very large values, membershiptest will be slow or even throw OutOfMemory exceptions.

#15660

in c you very often use enum1 & enum2 == enumx, this functionality might be wanted in Base as well

#2988

It'll be nice to not define the show methods on the concrete type directly since otherwise it cannot be easily redefined.

@StefanKarpinski
Copy link
Sponsor Member

It might be preferable to have an EnumSet{E<:Enum} type that lets you express sets of values of a given Enum type E. The integer values of e ∈ E would be used to indicate the bit offset for e in an EnumSet{E} collection. It seems to me that it makes sense to semantically distinguish between APIs that expect individual enum values and some set of enum values.

@Gnimuc
Copy link
Contributor

Gnimuc commented Apr 13, 2018

Any update on this?

@KristofferC
Copy link
Sponsor Member

The fact that you cant use the Julia enum to map into C enum is quite annoying when you want to generate a wrapper.

@visr
Copy link
Sponsor Contributor

visr commented Oct 11, 2018

Which of the above points still hold?

For C enums with multiple fields with the same value, I currently get by with just

@enum Fruit apple=1 orange=2
const kiwi = orange

@brenhinkeller brenhinkeller added the performance Must go faster label Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants