751554 - Support C++11 strongly-typed enumerations (enum class)

Status: RESOLVED FIXED

(Core :: MFBT, enhancement)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

http://en.wikipedia.org/wiki/C%2B%2B11#Strongly_typed_enumerations

This would allow catching extra type errors involving enums, like converting an enum to an integer type.  In the course of writing a patch for bug 751547, I tried converting the enum I was working on to an enum class and it found some interesting things, such as a method I hadn't converted from PRInt32 to nsEditor::OperationID.  With a regular enum, the compiler didn't care, because it silently converted the caller's OperationID to a PRInt32.  This might have masked other callers that passed invalid values.

GCC seems to support these since 4.4 or something, and VC is apparently adding support.  Of course, we'd need some kind of fallback.  Scoping might be an issue here -- the values in strongly-typed enums aren't put in the global scope.

I don't know how to go about implementing this, or whether it's the sort of thing we want to implement.  I just thought I'd file it as a wishlist feature.

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 1 -- Support strongly-typed enums in MFBT

Tell me if I should ask someone else to review this instead of you.

The workaround I have here for compilers that don't support enum class is fairly elaborate, but it works about as well as I think I can get it to work.  I *think* any code that will work on compilers that do support enum class will still work on those that don't -- the version for old compilers *should* be the same except with less type safety.  I think.

The fact that we need an implicit conversion to the inner enum for switch() to work, but can't have a deleted implicit conversion to int because that would make switch() ambiguous, and we have no way to get rid of the implicit conversion from enum to numeric types, means that the fallback has considerably less type safety than a real enum class.  I don't think there's any way to fix this.  I tried deleting as many operators as I could to catch some errors, but things like returning an enum value instead of an int will still work.

I was somewhat unsure about naming conventions here.  Probably what I have is wrong, because MFBT uses a different convention from Gecko AFAICT.  Tell me what I should change things to.

To test this out, I converted the nsEditor::OperationID enum to use the new macros.  It has a bunch of uses, so it's decent coverage of the things people might want to do with an enum, and it caught a bunch of problems with my definitions that I fixed.  Patches that do this conversion are to be attached, and I have a try run for the series: <https://tbpl.mozilla.org/?tree=Try&rev=d6d3c90e698a> (it might fail to build for all I know, so far I only know it works locally)

My ultimate aim here is to make nsresult an enum class, after I finish making it an enum (bug 777292).  This will catch additional bugs like attempting to return NS_OK from a function that returns a boolean, etc.

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 3 -- Fix OperationID type errors

OperationID isn't used so much, so this is all that's needed to get it to work.  The casts here between PRInt32 and OperationID are actually not necessarily safe in theory -- no underlying size is specified for OperationID, and the largest value is less than 64K, so some compilers might make it 16 bits.  Then the cast from OperationID to PRInt32 is still safe, but the reverse cast will truncate the value.  Which is fine, because any legitimate value will be <64k anyway, so actually this isn't a real bug.  But it's nice that this forces us to actually make the cast explicit so we're more likely to think about whether it's safe.

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 3 -- Make nsEditor::OperationID an enum class

Almost all just renaming to account for the different scoping rules.  Note the operator! I added -- there's no reason if (action) should stop working.  I changed kOpHTMLPaste to htmlPaste so it would start with lowercase, but left kOpLoadHTML as loadHTML.  I *think* everything other than nsEditor.h is changed using sed, so there's nothing else you need to specifically look out for.

(While I'm changing every mention of OperationID anyway, should we change its name, by any chance?  Maybe EAction or something?  We could put it in a mozilla::editor:: namespace and make the name as generic as we like; it's used quite a bit.)

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 647920 [details] [diff] [review]
Patch part 1 -- Support strongly-typed enums in MFBT

(Please disregard my OCD.)

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I just noticed that scoped enumerations always have a fixed underlying type, which defaults to "int".  This isn't good for us -- we probably want to always specify the underlying type.  Maybe a second argument to the macro?  Adding a separate macro like MOZ_ENUM_TYPE would be helpful, so you could do

  enum Foo MOZ_ENUM_TYPE(PRUint16)

and it would become either "enum Foo : PRUint16" or "enum Foo " depending on compiler support.  This would be nice to have separately because VC++ has supported this as a proprietary extension for a long time, at least since 2005 <http://msdn.microsoft.com/en-us/library/tzz3794d(v=vs.80).aspx>, so it could be used in regular old enums even with no enum class support.

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 647920 [details] [diff] [review]
Patch part 1 -- Support strongly-typed enums in MFBT

Clearing review requests, try is extremely orange.

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 1 -- Support explicit underlying enum types in MFBT

Per comment 5.

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 2, v2 -- Support strongly-typed enums in MFBT

Same as old part 1, described in comment 1, except updated to make use of underlying types.  I made them a mandatory argument so I wouldn't have to deal with variadic macro arguments.  Note that C++11 says that scoped enums with no explicit underlying type have an underlying type of "int", unlike unscoped enums, where it's not fully defined.  So if you want to specify a value above 0x7fffffff, for instance, as for nsresult, you have to specify the type.  I don't see it as a big deal that users have to specify the underlying type here, but if you want I can try to poke at MOZ_ASSERT and see how to get varargs to work so it's optional.

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 647922 [details] [diff] [review]
Patch part 3 -- Fix OperationID type errors

This didn't have to change.

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 4, v2 -- Make nsEditor::OperationID an enum class

Same as old patch 3, described in comment 3, except updated for the new need to specify an underlying type (fixing the issue mentioned in comment 2), plus the minor detail of

  inline bool operator!(const OperationID& aOp)
  {
-   return aOp != OperationID::none;
+   return aOp == OperationID::none;
  }

which I kind of suspect caused my try failures!  New try: https://tbpl.mozilla.org/?tree=Try&rev=67a6c0eab97e

Comment 11

[Joshua Cranmer [:jcranmer]]

(In reply to :Aryeh Gregor from comment #7)
> Created attachment 648300 [details] [diff] [review]
> Patch part 1 -- Support explicit underlying enum types in MFBT
> 
> Per comment 5.


AIUI, the only compiler that we support that doesn't support underlying type is gcc 4.2 on mac?

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I think so, yes, if by "support" you mean "use on tinderboxen".  (Are there official supported compiler versions other than that?)

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 648302 [details] [diff] [review]
Patch part 2, v2 -- Support strongly-typed enums in MFBT

I"m clearing the review request not because I object to the code in any way, but rather because I don't understand this feature well enough to review this impl.

Ehsan or Rafael, one of you feel comfortable reviewing this?

Comment 14

[(no longer active)]

Comment on attachment 648302 [details] [diff] [review]
Patch part 2, v2 -- Support strongly-typed enums in MFBT

Very nice!

Comment 15

[(no longer active)]

Comment on attachment 648305 [details] [diff] [review]
Patch part 4, v2 -- Make nsEditor::OperationID an enum class

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

I'm fine with a better name, however I don't think we should use a nested editor namespace.  You can go with EditorAction, I think, which is a much better name than OperationID (even though it's not shorter.)

r=me with or without that change -- I'm sort of ambivalent on that.

Comment 16

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/62a5c7e3be90
https://hg.mozilla.org/integration/mozilla-inbound/rev/1731751746ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec30d0346e92
https://hg.mozilla.org/integration/mozilla-inbound/rev/6470b9cd490f

(In reply to Ehsan Akhgari [:ehsan] from comment #15)
> I'm fine with a better name, however I don't think we should use a nested
> editor namespace.  You can go with EditorAction, I think, which is a much
> better name than OperationID (even though it's not shorter.)
> 
> r=me with or without that change -- I'm sort of ambivalent on that.

Didn't think it was worth it to make the change and re-test it if you're ambivalent.  I'd suggest EditAction, FWIW, which is one letter shorter.

I'll post on dev.platform now.

Comment 17

[(no longer active)]

(In reply to comment #16)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/62a5c7e3be90
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1731751746ea
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ec30d0346e92
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6470b9cd490f
> 
> (In reply to Ehsan Akhgari [:ehsan] from comment #15)
> > I'm fine with a better name, however I don't think we should use a nested
> > editor namespace.  You can go with EditorAction, I think, which is a much
> > better name than OperationID (even though it's not shorter.)
> > 
> > r=me with or without that change -- I'm sort of ambivalent on that.
> 
> Didn't think it was worth it to make the change and re-test it if you're
> ambivalent.  I'd suggest EditAction, FWIW, which is one letter shorter.

EditAction is good too!  r=me on a patch which renames this to make things easier for you.  :-)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/62a5c7e3be90
https://hg.mozilla.org/mozilla-central/rev/1731751746ea
https://hg.mozilla.org/mozilla-central/rev/ec30d0346e92
https://hg.mozilla.org/mozilla-central/rev/6470b9cd490f

Comment 19

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Ehsan Akhgari [:ehsan] from comment #17)
> EditAction is good too!  r=me on a patch which renames this to make things
> easier for you.  :-)

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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