952785 - relax GCC version requirements for MOZ_ENUM_TYPE

Status: RESOLVED WORKSFORME

(Core :: MFBT, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: relax GCC version requirements for MOZ_ENUM_TYPE

My local GCC 4.4 supports |enum foo : uint32_t|, and the status page at
https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code says
that GCC 4.4 ought to be good enough.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8350936 [details] [diff] [review]
relax GCC version requirements for MOZ_ENUM_TYPE

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

Hmm.  So really, when we added the enum support, we probably should have had two separate ifdefs for strong enums and typed enums.  Even if the conditions were the same, the rationales were distinct, and if reasons changed (as they did), better for them to evolve unsynchronized without any extra disentangling.  An interesting thing to consider when reviewing stuff in the future.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Hmm.  So really, when we added the enum support, we probably should have had
> two separate ifdefs for strong enums and typed enums.  Even if the
> conditions were the same, the rationales were distinct, and if reasons
> changed (as they did), better for them to evolve unsynchronized without any
> extra disentangling.  An interesting thing to consider when reviewing stuff
> in the future.

I'm not clear on what you are saying here...we have separate #defines for typed enums and strong enums already, don't we?

FWIW, light experimentation shows that GCC 4.4 on my machine also seems to support strong enums, so we could really just relax the requirements for both.  But for my particular use-case that motivated this patch, the typed enum support was really what I cared about.

Comment 4

[Jeff Walden [:Waldo]]

For the non-MSVC arm, it started as

if gcc is at least some version
  we have strong enums
  we have typed enums
end

Then it changed to

if gcc is at least (more-recent) version
  we have strong enums
  we have typed enums
end

in bug 833254, because the more-recent version was the first with a C++11 semantics fix for strong enums (only).

Then it changed again, the same way, in bug 923170, for the same reasons.

But really, many to most of those changes only apply to enum classes.  So if anyone had been thinking, after the first version, the single if controlling both instances would have changed to multiple ifs.  Or, if we'd had separate ifs the entire time, we wouldn't have conflated these, requiring this eventual reversion.  That's the point -- we should have had

if gcc is at least some version
  we have strong enums
end
if gcc is at least some version
  we have typed enums
end

from the start, in which case those other fixes would have naturally only affected one thing, not both.  Or at least people would have given it more thought when making changes, rather than seemingly-needlessly bumping the requirement for both in lockstep.

Incidentally, those bugs above are the reason not to relax the requirements for both.

Comment 5

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e13634eceb2

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Backed out:

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

for failures like:

https://tbpl.mozilla.org/php/getParsedLog.php?id=32361471&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=32361496&tree=Mozilla-Inbound

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to :Ms2ger from comment #6)
> Backed out:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ebde72c4ea2b
> 
> for failures like:
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=32361471&tree=Mozilla-
> Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=32361496&tree=Mozilla-
> Inbound

what is this i don't even

How does that work?  MOZ_ENUM_TYPE isn't even used anywhere close to code that would be responsible for those reftests!

Comment 8

[Chris Peterson [:cpeterson]]

Attachment: part-0-check-bool-not-nsresult.patch

With nfroyd's MOZ_ENUM_TYPE patch, nsresult becomes a real enum and gcc reports the following warning for B2G ICS:

dom/bluetooth/bluez/linux/BluetoothDBusService.cpp: In member function 'virtual void CreatePairedDeviceInternalTask::Run()':
dom/bluetooth/bluez/linux/BluetoothDBusService.cpp:2680: error: cannot convert 'bool' to 'nsresult' in initialization

This warning might have found a serious problem because this bool/nsresult mix-up would cause the following FIXME code to be skipped (because a non-zero `success` would be treated as an error nsresult and return early):

https://hg.mozilla.org/mozilla-central/annotate/ecf20a2484b6/dom/bluetooth/bluez/linux/BluetoothDBusService.cpp#l2680

Comment 9

[Chris Peterson [:cpeterson]]

With this B2G patch, nfroyd's MOZ_ENUM_TYPE patch is green on the try server:

https://tbpl.mozilla.org/?tree=Try&rev=cf39f6f80e33

btw, the B2G warning was a regression from bug 964817.

Comment 10

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8373112 [details] [diff] [review]
part-0-check-bool-not-nsresult.patch

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

It's a regression of Bug 964817. Thanks for fixing this.

Comment 11

[Chris Peterson [:cpeterson]]

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

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to Chris Peterson (:cpeterson) from comment #9)
> With this B2G patch, nfroyd's MOZ_ENUM_TYPE patch is green on the try server:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=cf39f6f80e33

The ICS Emulator R6 and R10 tests are still perma-orange:

https://tbpl.mozilla.org/?tree=Try&rev=c86fce45ae78

(R6 retriggers not done yet, but I'd bet they're going to be orange.)

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Nathan Froyd [AFK through July 21st] (:froydnj) from comment #3)
> FWIW, light experimentation shows that GCC 4.4 on my machine also seems to
> support strong enums, so we could really just relax the requirements for
> both.  But for my particular use-case that motivated this patch, the typed
> enum support was really what I cared about.

FWIW, mozilla::pkix has its own macro that is used for strongly-typed enums with type specifiers, which builds on all platforms, and which is defined like this:

  #if defined(_MSC_VER) && (_MSC_VER < 1700)
  // Microsoft added support for "enum class" in Visual C++ 2012. Before that,
  // Visual C++ has supported typed enums for longer than that, but using typed
  // enums results in C4480: nonstandard extension used: specifying underlying
  // type for enum.
  #define MOZILLA_PKIX_ENUM_CLASS  __pragma(warning(suppress: 4480)) enum
  #else
  #define MOZILLA_PKIX_ENUM_CLASS enum class
  #endif

and used like this:

    MOZILLA_PKIX_ENUM_CLASS DigestAlgorithm
    {
     [...]
    };

and:

    MOZILLA_PKIX_ENUM_CLASS KeyUsage : uint8_t
    {
      [...]
    };

This code is checked into mozilla-central since before Firefox 30. Consequently, it is safe to say that all our compilers in use except MSVC 2010 supports enum class and typed enums.

Comment 15

[Nathan Froyd [:froydnj]]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #14)
> This code is checked into mozilla-central since before Firefox 30.
> Consequently, it is safe to say that all our compilers in use except MSVC
> 2010 supports enum class and typed enums.

Yes.  But we had test failures when unconditionally enabling these bits, see comment 6 and comment 13.

I suppose now that several more months have passed, it's possible the failures have magically gone away...

Comment 16

[Nathan Froyd [:froydnj]]

Try failures are still there, just have moved around to some different suites:

https://tbpl.mozilla.org/?tree=Try&rev=204c6a885ca9

Comment 17

[Nathan Froyd [:froydnj]]

Attachment: relax GCC version requirements for MOZ_ENUM_TYPE

Rebased patch given some TypedEnum.h changes.

Comment 18

[Nathan Froyd [:froydnj]]

Typed enums are a thing for all the compilers we care about.