Closed Bug 1371771 Opened 7 years ago Closed 7 years ago

Add a facility for simultaneously declaring an enum and a count of its enumerators

Categories

(Core :: MFBT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: botond, Assigned: botond)

References

Details

Attachments

(1 file, 2 obsolete files)

Enumerations sometimes change over time, with enumerators being added, removed, reordered, and so on.

This poses a challenge for things like range checking and serialization [1], where we wish to know the enumeration's range of valid values.

Assuming an enum's values are contiguous and start from zero, knowing the range of valid values just requires knowing the highest valid value. However, the highest valid value can change over time, so we're forced to choose between one of two suboptimal things:

  (1) Store the highest valid value in a variable or something
      that potentially needs to be updated any time the
      enumeration is updated. 

      This is suboptimal because having to remember to keep
      two things in sync is error-prone.

  (2) Add a "sentinel" value to the enumeration, with the
      understanding that that's always "one past" the highest
      valid value, since new values will be added before it.

      This is suboptimal because now the sentinel value needs
      to be handled in switch statements to avoid compiler
      warnings and such.

We've so far mostly been doing (2).

It would be nice if we had a way to derive the highest valid value (or, equivalently for a contiguous enum, the number of enumerators) just from the definition of the enumeration itself.

Reflection (which we'll get in C++20 or beyond) will give us a way to do that, but until then, we can employ a macro-based approach, where we express an enum definition like so:

  DEFINE_ENUM(MyEnumType, (EnumeratorA, EnumeratorB, EnumeratorC));

and get that to expand to:

  enum MyEnumType { EnumeratorA, EnumeratorB, EnumeratorC };
  int MyEnumType_Count = 3;

Then, any time we change the enumeration, the corresponding _Count variable is automatically updated.

This bug tracks adding such a DEFINE_ENUM facility.

[1] http://searchfox.org/mozilla-central/rev/a798ee4fc323f9387b7576dbed177859d29d09b7/ipc/glue/IPCMessageUtils.h#194
Thank you Botond for proposing this.

If I may suggest a little bit of feature creep:
It would be great to allow the creation of `enum class` and `enum struct`, and the optional base type (e.g. `enum : uint8_t {...};`).

And a bit of bike-shedding about how the count is provided:
How about a more C++-y type-traits style?
E.g.: We have one global `template <typename T> struct EnumTraits;` that gets specialized by DEFINE_ENUM: `template <> struct EnumTraits <MyEnumType> { using BaseType = uint32_t; static constexpr BaseType count = 3; };`.
This would help with adding features later on, e.g., an enum-to-string function, which would be very nice for logging&debugging.

(I haven't looked at the reflection proposal, maybe there would be better ideas we could lift from it?)
(In reply to Gerald Squelart [:gerald] from comment #1)
> If I may suggest a little bit of feature creep:
> It would be great to allow the creation of `enum class` and `enum struct`,
> and the optional base type (e.g. `enum : uint8_t {...};`).

Yup, good idea.

> And a bit of bike-shedding about how the count is provided:
> How about a more C++-y type-traits style?
> E.g.: We have one global `template <typename T> struct EnumTraits;` that
> gets specialized by DEFINE_ENUM: `template <> struct EnumTraits <MyEnumType>
> { using BaseType = uint32_t; static constexpr BaseType count = 3; };`.

Agreed, this is definitely better.

> (I haven't looked at the reflection proposal, maybe there would be better
> ideas we could lift from it?)

Out of curiosity, I looked up how this would look with the proposed reflection facilities:

  enum MyEnum {a, b, c};

  using namespace std::experimental::reflect;
  const int count = get_size_v<get_enumerators_t<reflexpr(MyEnum)>>;
  
(Note: this is just a proposed syntax, it may not be what ends up being standardized.)
One downside of the trait approach is that it means you can't use the macro to define a class-scope enumeration (since the trait specialization needs to be at namespace scope), which is unfortunate because many of our enumerations are at class scope.
Assignee: nobody → botond
This is an example usage, for ScrollWheelInput::ScrollDeltaType.

Kats, how do you feel about this approach for axing sentinels?
Attachment #8881194 - Flags: feedback?(bugmail)
Comment on attachment 8881194 [details] [diff] [review]
Part 2 - Example usage for ScrollWheelInput::ScrollDeltaType

Review of attachment 8881194 [details] [diff] [review]:
-----------------------------------------------------------------

I like it!
Attachment #8881194 - Flags: feedback?(bugmail) → feedback+
Attachment #8881192 - Attachment is obsolete: true
Attachment #8881194 - Attachment is obsolete: true
Blocks: 1377020
I cleaned up the implementation and posted it for review. Bug 1377020 contains an example of usage.
I forgot to mention that I didn't go with the trait approach due to the issue described in comment 3 (not being able to do it for class-scope enumerations). I could have done it for namespace-scope enumerations, but I wanted the interface to be consistent between class-scope and namespace-scope enumerations. We can still add traits for namespace-scope enumerations in a follow-up, if desired.
Makes sense; thank you for considering it.
Comment on attachment 8882052 [details]
Bug 1371771 - Add a MOZ_DEFINE_ENUM macro and variants to MFBT.

https://reviewboard.mozilla.org/r/153132/#review158836

r-'ing not because I think this is a bad idea, but because there's a bit of refinement that could be done.

::: mfbt/DefineEnum.h:7
(Diff revision 1)
> +/*
> + * MOZ_DEFINE_ENUM allows simultaneously defining an enumeration and a constant
> + * that stores the count of its enumerators.
> + *
> + * This avoids having to add a "sentinel" enumerator for range checking
> + * purposes.
> + */

Maybe just `// Poor man's reflection for enumerations`. :)  This comment just seems redundant with the larger comment below and having a single-line comment would provide a nicer view in dxr/searchfox in any event.

::: mfbt/DefineEnum.h:33
(Diff revision 1)
> + * MOZ_DEFINE_ENUM(aEnumName, aEnumerators) is a macro that allows
> + * simultaneously defining an enumeration named |aEnumName|, and a constant
> + * that stores the number of enumerators it has.

I realize this is kind of trivial code, but it'd be great if we had some `static_assert` tests somewhere to verify that all of these macros generated what you claim in this comment.

::: mfbt/DefineEnum.h:39
(Diff revision 1)
> + * simultaneously defining an enumeration named |aEnumName|, and a constant
> + * that stores the number of enumerators it has.
> + *
> + * The motivation is to allow the enumeration to evolve over time without
> + * either having to manually keep such a constant up to date, or having to
> + * add a special "sentinel" enumerator for this purpose.

Maybe explain that even though a sentinel enum is trivial to add, its existence causes problems with e.g. `switch` statements and the like?

::: mfbt/DefineEnum.h:41
(Diff revision 1)
> + * |aEnumerators| is expected to be a comma-separated list of enumerator
> + * names, enclosing in parentheses.

Nit: I think "...enclosed in parentheses" is more correct.

How much should we care that the parenthesized list is technically permitted to be anything that an `enum {...}` declaration would accept?  E.g. the code as written will permit:

```
MOZ_DEFINE_ENUM(MyEnum, (Foo = 1, Bar = -2, ...));
```

and similar things.  And that would throw off the `kHighestMyEnum` value...

I guess we can't verify that the defined enum isn't a signed type, because the implementation is permitted to use signed types even if the enumeration contains all unsigned values and the chosen signed type is large enough. :(

If we do care about this...can you think of any way to attempt to try and complain if people do this?  At the very least, the documentation should point out this potential loophole and tell people to Not Do That.

::: mfbt/DefineEnum.h:78
(Diff revision 1)
> + *    - An |_AT_CLASS_SCOPE| variant, designed for enumerations defined
> + *      at class scope. For these, the generated constants are static,
> + *      and have names prefixed with "s" instead of "k" as per
> + *      naming convention.
> + *
> + *  (and combinations of these).

It is unfortunate that we have such an impoverished macro system as to need separate variants for all of these.
Attachment #8882052 - Flags: review?(nfroyd) → review-
(In reply to Nathan Froyd [:froydnj] from comment #11)
> ::: mfbt/DefineEnum.h:7
> (Diff revision 1)
> > +/*
> > + * MOZ_DEFINE_ENUM allows simultaneously defining an enumeration and a constant
> > + * that stores the count of its enumerators.
> > + *
> > + * This avoids having to add a "sentinel" enumerator for range checking
> > + * purposes.
> > + */
> 
> Maybe just `// Poor man's reflection for enumerations`. :)  This comment
> just seems redundant with the larger comment below and having a single-line
> comment would provide a nicer view in dxr/searchfox in any event.

Done.

> ::: mfbt/DefineEnum.h:33
> (Diff revision 1)
> > + * MOZ_DEFINE_ENUM(aEnumName, aEnumerators) is a macro that allows
> > + * simultaneously defining an enumeration named |aEnumName|, and a constant
> > + * that stores the number of enumerators it has.
> 
> I realize this is kind of trivial code, but it'd be great if we had some
> `static_assert` tests somewhere to verify that all of these macros generated
> what you claim in this comment.

I added mfbt/tests/TestDefineEnum.cpp with some static_asserts.

> ::: mfbt/DefineEnum.h:39
> (Diff revision 1)
> > + * simultaneously defining an enumeration named |aEnumName|, and a constant
> > + * that stores the number of enumerators it has.
> > + *
> > + * The motivation is to allow the enumeration to evolve over time without
> > + * either having to manually keep such a constant up to date, or having to
> > + * add a special "sentinel" enumerator for this purpose.
> 
> Maybe explain that even though a sentinel enum is trivial to add, its
> existence causes problems with e.g. `switch` statements and the like?

Done.

> ::: mfbt/DefineEnum.h:41
> (Diff revision 1)
> > + * |aEnumerators| is expected to be a comma-separated list of enumerator
> > + * names, enclosing in parentheses.
> 
> Nit: I think "...enclosed in parentheses" is more correct.

Fixed.

> How much should we care that the parenthesized list is technically permitted
> to be anything that an `enum {...}` declaration would accept?  E.g. the code
> as written will permit:
> 
> ```
> MOZ_DEFINE_ENUM(MyEnum, (Foo = 1, Bar = -2, ...));
> ```
> 
> and similar things.  And that would throw off the `kHighestMyEnum` value...
> 
> I guess we can't verify that the defined enum isn't a signed type, because
> the implementation is permitted to use signed types even if the enumeration
> contains all unsigned values and the chosen signed type is large enough. :(
> 
> If we do care about this...can you think of any way to attempt to try and
> complain if people do this?  At the very least, the documentation should
> point out this potential loophole and tell people to Not Do That.

This is a very good point!

I found a way to disallow enumerators with initializers altogether, and I do so in the updated patch. I couldn't think of a way to prevent just problematic (out-of-range) initializers, but I don't see all that much value in allowing those to begin with.

> ::: mfbt/DefineEnum.h:78
> (Diff revision 1)
> > + *    - An |_AT_CLASS_SCOPE| variant, designed for enumerations defined
> > + *      at class scope. For these, the generated constants are static,
> > + *      and have names prefixed with "s" instead of "k" as per
> > + *      naming convention.
> > + *
> > + *  (and combinations of these).
> 
> It is unfortunate that we have such an impoverished macro system as to need
> separate variants for all of these.

We could try to "overload" the same macro name and express the variation in the arguments only, but I feel like that would both complicate the implementation, and make the interface less user-friendly (e.g. it could lead to hard-to-understand errors if you misuse the interface).
Here's a Try push for the patch (also including the patches from bug 1377020 which contain real-life uses):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=da3d40ca975230bca7cffc3022708ef728604e29

And here's a Try push with CONFIRM_COMPILATION_ERRORS defined in TestDefineEnum.cpp, to demonstrate that the compiler catches an attempt to use an initializer for an enumerator:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8587a50a4944ece042d8df41899056f37893e8a
Comment on attachment 8882052 [details]
Bug 1371771 - Add a MOZ_DEFINE_ENUM macro and variants to MFBT.

https://reviewboard.mozilla.org/r/153132/#review159198

Thank you!

::: mfbt/DefineEnum.h:96
(Diff revision 2)
> + * If |aEnumeratorDecl| is just the enumerator name without an identifier,
> + * this expression compiles fine. However, if |aEnumeratorDecl| includes an
> + * initializer, as in |eEnumerator = initializer|, then this will fail to
> + * compile in expression context, since |eEnumerator| is not an lvalue.

Nice!

::: mfbt/tests/moz.build:24
(Diff revision 2)
>      'TestCasting',
>      'TestCeilingFloor',
>      'TestCheckedInt',
>      'TestCountPopulation',
>      'TestCountZeroes',
> +    'TestDefineEnum',

Because reasons, this test also needs to be added to  `testing/cppunittest.ini`...though I don't know if it'll matter much, since the test is basically compile-time only anyway.  Better to be consistent and just add the test in both places, I think.
Attachment #8882052 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #15)
> Comment on attachment 8882052 [details]
> ::: mfbt/tests/moz.build:29
> (Diff revision 2)
> > +    'TestDefineEnum',
> 
> Because reasons, this test also needs to be added to 
> `testing/cppunittest.ini`...though I don't know if it'll matter much, since
> the test is basically compile-time only anyway.  Better to be consistent and
> just add the test in both places, I think.

Wow, that's scary!

At a glance, I see a few files in mfbt/tests/moz.build that are not in cppunittest.ini:
TestBufferList, TestNotNull, TestRange, TestResult, TestSmallPointerArray
Should they be added too?

Should we have some automation to verify that we don't forget such things in the future? -- Apart from "r?froydnj" :-)
(In reply to Gerald Squelart [:gerald] from comment #16)
> Wow, that's scary!
> 
> At a glance, I see a few files in mfbt/tests/moz.build that are not in
> cppunittest.ini:
> TestBufferList, TestNotNull, TestRange, TestResult, TestSmallPointerArray
> Should they be added too?
> 
> Should we have some automation to verify that we don't forget such things in
> the future? -- Apart from "r?froydnj" :-)

Yes.  I believe ahal has been working on setting up some sort of linting framework wherein we might perform things like this.  Might be worth filing a bug and needinfo'ing him to see if such a thing is feasible.
(In reply to Nathan Froyd [:froydnj] from comment #15)
> ::: mfbt/tests/moz.build:24
> (Diff revision 2)
> >      'TestCasting',
> >      'TestCeilingFloor',
> >      'TestCheckedInt',
> >      'TestCountPopulation',
> >      'TestCountZeroes',
> > +    'TestDefineEnum',
> 
> Because reasons, this test also needs to be added to 
> `testing/cppunittest.ini`...though I don't know if it'll matter much, since
> the test is basically compile-time only anyway.  Better to be consistent and
> just add the test in both places, I think.

Added.
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cb0262ab95fa
Add a MOZ_DEFINE_ENUM macro and variants to MFBT. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/cb0262ab95fa
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.