835648 - Move typed enum macros into a separate header

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch

Attributes.h is a bit unwieldy, and the enum stuff isn't really attributes, nor does it need to be foisted on everyone wanting attribute stuff.  This also makes it much more obvious where this stuff lives -- "attributes" only vaguely applied at best, to someone skimming MXR listings.

As with all mfbt patches I write, I will tryserver this before landing it, should it pass review.

Comment 1

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

Comment on attachment 707395 [details] [diff] [review]
Patch

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

It should.

::: content/media/webaudio/PannerNode.h
@@ +13,1 @@
>  #include "mozilla/ErrorResult.h"

Other order

Comment 2

[Jeff Walden [:Waldo]]

https://tbpl.mozilla.org/?tree=Try&rev=39111ac333b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/3eb633a97c1f

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Backed out for turning Windows mochitests orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5bdb6d94a5d

Comment 4

[Jeff Walden [:Waldo]]

...and only Mochitests.  Building was successful, other test suites did fine, including ones that display browser UI and stuff.  Going to do a Windows build locally and hope I can debug this.

I hate MSVC.

Comment 5

[Jeff Walden [:Waldo]]

It turns out the reason for the bustage was that the code that defines the nsresult type does some really dodgy things, testing for the support macros in TypedEnum.h being defined.  (Those aren't API, and they shouldn't be tested!  Although for the moment I'm unsure how to work around that, for the particular case in question.)

When those tests failed -- I hadn't known of these uses or searched for them, so I didn't add a TypedEnum.h #include to nsError.h -- we fell back to defining nsresult as a plain old enum.  But this resulted in the enum having signed-integer type, not unsigned-integer type.  (Yes, our fallback code is buggy.  I guess we never compile anywhere with a compiler that doesn't support enum classes or enums with types, and we don't have any tinderboxen doing so.)  The code that implements Components.results.FOO has a JS_NumberValue(double(rv)) in it, and when rv changed to signed, the resulting number became negative, and a comparison against that value failed.

The nsresult code should be fixed so the fallback case isn't buggy, of course, but adding the #include is good enough for this bug's purposes, for now.

https://tbpl.mozilla.org/?tree=Try&rev=898a1cd52439
https://hg.mozilla.org/integration/mozilla-inbound/rev/722643e05bb7

Comment 6

[(no longer active)]

(In reply to comment #5)
> It turns out the reason for the bustage was that the code that defines the
> nsresult type does some really dodgy things, testing for the support macros in
> TypedEnum.h being defined.  (Those aren't API, and they shouldn't be tested! 
> Although for the moment I'm unsure how to work around that, for the particular
> case in question.)
> 
> When those tests failed -- I hadn't known of these uses or searched for them,
> so I didn't add a TypedEnum.h #include to nsError.h -- we fell back to defining
> nsresult as a plain old enum.  But this resulted in the enum having
> signed-integer type, not unsigned-integer type.  (Yes, our fallback code is
> buggy.  I guess we never compile anywhere with a compiler that doesn't support
> enum classes or enums with types, and we don't have any tinderboxen doing so.) 
> The code that implements Components.results.FOO has a
> JS_NumberValue(double(rv)) in it, and when rv changed to signed, the resulting
> number became negative, and a comparison against that value failed.

Sorry you had to go through this terribleness.  I have done the exact same debugging one time in a non-distant future...

> The nsresult code should be fixed so the fallback case isn't buggy, of course,
> but adding the #include is good enough for this bug's purposes, for now.

It may be a good idea to #undef macros which external code should not be relying on...

Comment 7

[Jeff Walden [:Waldo]]

(In reply to :Ehsan Akhgari from comment #6)
> It may be a good idea to #undef macros which external code should not be
> relying on...

Hmm, yes, that actually is doable, isn't it -- just check for defined(MOZ_ENUM_TYPE) and so on.  I'll update the other patch to do that (and #undef the other things, which usually I do but here didn't because I thought they had to exist, which they obviously don't at a saner time of day).

Comment 8

[Jeff Walden [:Waldo]]

Or, no, actually I couldn't do that, because the MOZ_ENUM_TYPE and MOZ_BEGIN_ENUM_CLASS stuff has emulation bits behind it.  Sigh.  I think leaving the current over-dependency in place for now at least is what we'll have to do, unless you have any bright ideas.  When we stop supporting gcc < 4.5.1, which may not be horribly far off, this mess can go away then.

Comment 9

[(no longer active)]

(In reply to comment #8)
> Or, no, actually I couldn't do that, because the MOZ_ENUM_TYPE and
> MOZ_BEGIN_ENUM_CLASS stuff has emulation bits behind it.  Sigh.  I think
> leaving the current over-dependency in place for now at least is what we'll
> have to do, unless you have any bright ideas.  When we stop supporting gcc <
> 4.5.1, which may not be horribly far off, this mess can go away then.

/me wishes for that day to come...

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/722643e05bb7