dropbox / djinni

A tool for generating cross-language type declarations and interface bindings.
Apache License 2.0
2.88k stars 487 forks source link

Countable Enums #349

Open steipete opened 6 years ago

steipete commented 6 years ago

This PR allows to query the size of an enum, without polluting the enum with a Count entry. (So switch-case expressions can still be complete)

I tried a lot of different approaches and settled on adding template specializers for std::numeric_limits.

We use it in our codebase to ensure enum<->string maps cover all enumeration cases:

Example use case:

#define PDFCAssertMapCoversAllEnumCases(var) PDFCAssert(var.size() == (size_t)std::numeric_limits<decltype(var)::mapped_type>::max() + 1);

(This could also be a static_assert, needs further changes on our maps though)

I do not know if this is the best approach, there are many very complex enum wrappers around, yet this variant is more minimal, compiles away completely and doesn't modify/pollute the enum itself.

Without specialization, std::numeric_limits does not make sense on enum types and returns 0. It's very unlikely that any code breaks because these specializations are added.

The second addition allows to call ++ on an enum to increment over it in a loop. Since we add a few such operators for flags, this seemed appropriate.

Tests passed locally:

Test Suite 'All tests' passed at 2018-01-11 23:22:01.121.
     Executed 50 tests, with 0 failures (0 unexpected) in 0.037 (0.094) seconds
** TEST SUCCEEDED **

     [java] .........
     [java] Time: 0.133
     [java]
     [java] OK (38 tests)
     [java]

BUILD SUCCESSFUL
Total time: 32 seconds
steipete commented 6 years ago

Thanks for looking into this! I can address the concerns. Static variables would work as well, ideally there's some mechanism where I can infer the limits from just knowing the enum name, without having to use macros to stitch together name+ Max() or similar - that was what ultimately moved me to use std::numeric_limits. I was also considering using a custom template-specializable function, however that has the drawback that we then need a shared header where the base definition is, currently there's none needed for these files. (e.g. color.hpp only includes <functional>)

artwyman commented 6 years ago

I can see the desire for something you can derive directly from the type for meta-programming purposes. With that in mind I think extending numeric_limits is reasonable. It's meant to be specialized for user-defined types, and since Djinni enums are their own domain of such types we can define what's expected on them, without leaking into users' expectation for other enums.

I the limits for a flags type need to go from 0 to 2^(N-1) by the nature of flags. My question about arithmatic operators also remain. Seems like adding additional operators to enums are orthogonal to this feature, so maybe should be approached in a separate PR if you want to agitate for them. I have some potential concerns about things like increment, though, since it can result in values are out of range if implemented in the expected way.

FWIW, adding a header wouldn't be a big deal. There's already the tiny djinni_common.hpp used in many places: https://github.com/dropbox/djinni/blob/master/support-lib/djinni_common.hpp