Countable Enums

[steipete]

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

[artwyman]

I made some comments inline with concerns about the specific implementation.

The overall approach seems reasonable on the surface, though it's somewhat complex, and I wonder how widely accepted it is to specialize numeric_limits in this way. What would be your concern with doing this with a pair of static constants instead?

[artwyman]

Needs to be outdented

[artwyman]

I don't think this calculation is correct for flags. A flag type like this may have 9 values, but its range is a full 9 bits.

[artwyman]

Why are you adding increment, but not decrement or any other arithmatic operations? Feels like a partial feature.

[thisismyworkgithub]

If the increment operator is being used as a way to iterate over a set of enum values, it may be more C++ idiomatic to define an enum_iterator class of some kind that follows a general set of rules to reach all values. Otherwise, operator++ would be sequencing through all combinations of valid flags when used on bit-flag enums, or possibly leading to illegal values (as previously mentioned by @artwyman)

[artwyman]

See my comment in the example generated code. I wonder why it would make sense to define only one such operator, rather than a full set.

Increment in particular has the issue that it can cause overflow/underflow and result in illegal values.

[steipete]

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]

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