918436 - Don't require that the underlying type be specified, in MOZ_BEGIN_ENUM_CLASS and MOZ_BEGIN_NESTED_ENUM_CLASS

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Daniel Holbert [:dholbert]]

Right now, our MOZ_BEGIN_ENUM_CLASS() macro requires you to specify the underlying type (e.g. int, int32_t, uint16_t, whatever).

In many (most?) cases, when we're defining an enum (or upgrading an existing "enum { ... }" definition, we don't really care about the underlying type - that's something we should allow developers to trust the compiler with.

Waldo says in IRC: "C++11 at least says |enum class Foo { ... };| is valid", so I'd like for us to add a macro like MOZ_BEGIN_ENUM_CLASS that expands to that in compilers that support C++11.

(Maybe MOZ_BEGIN_UNTYPED_ENUM_CLASS?  Or we could call it MOZ_BEGIN_ENUM_CLASS, and rename the existing macro to MOZ_BEGIN_TYPED_ENUM_CLASS.)

Comment 1

[Daniel Holbert [:dholbert]]

I imagine this would live in TypedEnum.h... though that'd be a slightly odd place for it, because I believe the "Typed" in that header's name means "letting you ask for the underlying type", which this new macro would explicitly *not* do. Still, that's our only MFBT enum-related header, so it probably makes the most sense.

Comment 2

[Jeff Walden [:Waldo]]

Yeah, that header.

Actually we don't need a new macro for this -- we can just make the type argument to MOZ_BEGIN_ENUM_CLASS optional.  (I think it comes second, too lazy to check.)  Anyone who wants a type isn't going to assume not specifying a type will do anything, anyone who doesn't want one gets whatever.  Just a matter of working with C++11 variadic macro stuff a bit (see MOZ_ASSERT for how you might do it).

Comment 3

[Biru [:poiru]]

Attachment: Make the type argument of MOZ_BEGIN(_NESTED)_ENUM_CLASS optional

This copies the macro arg count logic from Assertions.h. It would be possible to centralize parts of it (the MOZ_COUNT*(), MOZ_..._GLUE() macros, at least) into a new header, but I didn't do that now. If needed, I can move the shared bits into e.g. MacroArgs.h.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 808272 [details] [diff] [review]
Make the type argument of MOZ_BEGIN(_NESTED)_ENUM_CLASS optional

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

glandium tried to centralize this macro logic, once.  It's apparently fairly difficult to do because of MSVC's bugginess wrt __VA_ARGS__.  I'm fine (well, fine enough) not worrying about it.

I'm holding this back for lack of a test that ensures this stuff compiles, because I don't fully trust it to have worked around MSVC's bugginess exactly, without guaranteed users of it.  With that, this is good to go.

::: mfbt/TypedEnum.h
@@ +265,5 @@
> +   /* The actual macro. */
> +#  define MOZ_BEGIN_NESTED_ENUM_CLASS_GLUE(x, y) x y
> +#  define MOZ_BEGIN_NESTED_ENUM_CLASS(...) \
> +     MOZ_BEGIN_NESTED_ENUM_CLASS_GLUE(MOZ_BEGIN_NESTED_ENUM_CLASS_CHOOSE_HELPER( \
> +                                      MOZ_COUNT_BEGIN_ENUM_CLASS_ARGS(__VA_ARGS__)), \

Just have

MOZ_BEGIN_NESTED_ENUM_CLASS_CHOOSE_HELPER(MOZ_COUNT_BEGIN_ENUM_CLASS_ARGS(__VA_ARGS__)), \

all on a single line.  Adhering strictly to 8ch makes this less readable, so let's ignore it here.

@@ +268,5 @@
> +     MOZ_BEGIN_NESTED_ENUM_CLASS_GLUE(MOZ_BEGIN_NESTED_ENUM_CLASS_CHOOSE_HELPER( \
> +                                      MOZ_COUNT_BEGIN_ENUM_CLASS_ARGS(__VA_ARGS__)), \
> +                                      (__VA_ARGS__))
> +
> +#  define MOZ_BEGIN_ENUM_CLASS(...) MOZ_BEGIN_NESTED_ENUM_CLASS(__VA_ARGS__)

Given my recollections of the MSVC bug with this stuff, I wouldn't be surprised to find out that having this delegation doesn't work.  Could you assuage my worries by adding a mfbt/tests/TestTypedEnum.cpp file (copy one of the other .cpp files there for structure), that contains a MOZ_BEGIN_ENUM_CLASS with and without type, and ditto for the nested-enum macro?  I'm uncomfortable relying on in-tree uses to detect bugs here, particularly when the in-tree uses could be in platform-specific code.

Comment 5

[Biru [:poiru]]

Attachment: Make the type argument of MOZ_BEGIN(_NESTED)_ENUM_CLASS optional and add tests

Updated patch with suggestions from comment #4.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=fe905c972dce

The Windows XP build in the try push above failed with an infra exception. Here is a retry for it: https://tbpl.mozilla.org/?tree=Try&rev=d6f4d901ed1c

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 811018 [details] [diff] [review]
Make the type argument of MOZ_BEGIN(_NESTED)_ENUM_CLASS optional and add tests

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

::: mfbt/tests/TestTypedEnum.cpp
@@ +34,5 @@
> +  // Simply check that this compiles.
> +  AutoEnum autoEnum = AutoEnum::A;
> +  CharEnum charEnum = CharEnum::B;
> +  Nested::AutoEnum nestedAutoEnum = Nested::AutoEnum::C;
> +  Nested::CharEnum nestedCharEnum = Nested::CharEnum::D;

It's kinda tempting fate to not actually use any of these variables.  How about you make them globals instead, so they can't be deemed unused?

::: mfbt/tests/moz.build
@@ +16,5 @@
>      'TestFloatingPoint.cpp',
>      'TestIntegerPrintfMacros.cpp',
>      'TestSHA1.cpp',
>      'TestTypeTraits.cpp',
> +    'TestTypedEnum.cpp',

D comes before T, so move this before TestTypeTraits.cpp.

Comment 7

[Jeff Walden [:Waldo]]

Birunthan, do you need me to check in the patch here?  (With the requested changes, except probably not the latter, because I'm told mach may order uppercase before earlier lowercase.  I'll test before landing if needed.)

Comment 8

[Biru [:poiru]]

Attachment: Make the type argument of MOZ_BEGIN(_NESTED)_ENUM_CLASS optional and add tests

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> Comment on attachment 811018 [details] [diff] [review]
>
> It's kinda tempting fate to not actually use any of these variables.  How
> about you make them globals instead, so they can't be deemed unused?

Updated the patch for this.

> D comes before T, so move this before TestTypeTraits.cpp.

As you mentioned, mach does indeed match uppercase before lowercase.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> Birunthan, do you need me to check in the patch here?

Yep.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9a7d1fcb34c

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d9a7d1fcb34c