1111290 - replace TypedEnum.h usage with real enum classes

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Cameron McCormack (:heycam)]

According to https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code we should be able to use enum classes one we drop support for MSVC 2010.

Comment 1

[Jeff Walden [:Waldo]]

Note that GCC emits a warning (wrongly) if you use a typed enum (enum class as well?) as a bit field type, so be careful when making the switch.

Comment 2

[Cameron McCormack (:heycam)]

BTW I think we already should be able to use typed enums instead of MOZ_ENUM_TYPE (according to the above wiki page), but dropping MSVC 2010 support also lets us use enum classes instead of MOZ_BEGIN_ENUM_CLASS etc.  We'll want to keep MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS though.

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: Remove MOZ_(BEGIN|END)_ENUM_CLASS

Executed

convert 'MOZ_BEGIN_ENUM_CLASS(\([^,]*\),\s*\([^)]*\))' 'enum class \1 : \2 {'
convert 'MOZ_BEGIN_ENUM_CLASS(\([^,)]*\))' 'enum class \1 {'
convert 'MOZ_END_ENUM_CLASS([^,)]*)' '};'

on the tree. See bug 1118486 comment #1 about the convert function.

Comment 4

[Masatoshi Kimura [:emk]]

Attachment: Non-mechanical changes

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: Remove MOZ_HAVE_CXX11_STRONG_ENUMS and macro definitions

Comment 6

[Masatoshi Kimura [:emk]]

Try runs:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6a356960ad9
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c5f047a2d98

Comment 7

[Masatoshi Kimura [:emk]]

Attachment: Remove TypedEnum.h and fold TypedEnumInternal.h into TypedEnumBits.h

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

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 8554130 [details] [diff] [review]
Remove MOZ_(BEGIN|END)_ENUM_CLASS

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

It's a bit odd how many of these enums seem to be explicitly requesting int as the underlying type, given that's to some approximation what will be used by default in most situations.  Wonder why this code requests it so often...

::: mfbt/tests/TestTypedEnum.cpp
@@ +111,5 @@
>  MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS(Nested::AutoEnumBitField)
>  MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS(Nested::CharEnumBitField)
>  
>  #define MAKE_STANDARD_BITFIELD_FOR_TYPE(IntType)                   \
> +  enum class BitFieldFor_##IntType : IntType {             \

Realign here.

@@ +116,4 @@
>      A = 1,                                                         \
>      B = 2,                                                         \
>      C = 4,                                                         \
> +  };                        \

And here.

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 8554132 [details] [diff] [review]
Remove MOZ_HAVE_CXX11_STRONG_ENUMS and macro definitions

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

::: mfbt/TypedEnum.h
@@ -53,5 @@
>   *   };
>   *
>   *   MOZ_FINISH_NESTED_ENUM_CLASS(FooBar::Enum)
>   */
> -#if defined(MOZ_HAVE_CXX11_STRONG_ENUMS)

It's fine enough just removing this all for reviewing purposes.  But you should go back and unindent everything in this if-block, once you've done this.  And it looks like there's a bunch of comments documentation above that needs editing to remove a bunch of detritus.  Separate patch to do that, please?

::: mfbt/tests/TestTypedEnum.cpp
@@ +159,5 @@
>  TestNonConvertibilityForOneType()
>  {
>    using mozilla::IsConvertible;
>  
> +#if defined(MOZ_HAVE_EXPLICIT_CONVERSION)

I've reviewed a patch somewhere else that might have removed this last test as well -- you might be racing someone to land.  :-)

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 8554160 [details] [diff] [review]
Remove TypedEnum.h and fold TypedEnumInternal.h into TypedEnumBits.h

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

::: mfbt/tests/TestTypedEnum.cpp
@@ -10,1 @@
>  #include "mozilla/TypedEnumBits.h"

I am now grateful that I insisted on the bitwise operator additions to typed arrays being in a separate header from TypedEnum.h, back in the day -- no worries about anyone bootlegging those macros from TypedEnum.h.  :-)

Comment 11

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7d1736f58e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fab2faa7f9d
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f7f003696ad

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> It's fine enough just removing this all for reviewing purposes.  But you
> should go back and unindent everything in this if-block, once you've done
> this.  And it looks like there's a bunch of comments documentation above
> that needs editing to remove a bunch of detritus.  Separate patch to do
> that, please?

I folded the next patch instead :)

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e7d1736f58e1
https://hg.mozilla.org/mozilla-central/rev/2fab2faa7f9d
https://hg.mozilla.org/mozilla-central/rev/7f7f003696ad