Move typed enum macros into a separate header

RESOLVED FIXED in mozilla21

Status

()

defect
--
minor
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

Trunk
mozilla21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

7 years ago
Posted patch PatchSplinter Review
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.
Attachment #707395 - Flags: review?(Ms2ger)
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
Attachment #707395 - Flags: review?(Ms2ger) → review+
Assignee

Comment 4

7 years ago
...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.
Assignee

Comment 5

7 years ago
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

7 years ago
(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...
Assignee

Comment 7

7 years ago
(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).
Assignee

Comment 8

7 years ago
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

7 years ago
(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...
https://hg.mozilla.org/mozilla-central/rev/722643e05bb7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.