#define None in X11 causing problems

RESOLVED FIXED in Firefox 51

Status

()

Core
General
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: manishearth, Assigned: botond)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
There's a #define None in X11/X.h which often reaches other parts of the codebase. This means that any mention of the identifier "None" in code that has this transitively included will lead to incomprehensible errors, as seen in https://treeherder.mozilla.org/#/jobs?repo=try&revision=8458b0423ba2677e5f1da5b8617208cd5a837e35

Simply #undef'ing it (as done in https://dxr.mozilla.org/mozilla-central/source/dom/animation/ComputedTiming.h#14) does not always work, since there is code in Firefox that actually uses it (e.g. https://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLContextProviderGLX.cpp#308), and if your include happens to reach this code, you get errors (https://treeherder.mozilla.org/#/jobs?repo=try&revision=c40857cf2683&selectedJob=24362075)

Perhaps we should have a wrapper header that defines X11_None instead of None (same for some of the other common constants there), and exclusively use this header in our code? None is a pretty useful identifier to be able to use -- indeed, we already use it as an enum variant in many places in the codebase, places which have so far not hit this issue or have been able to safely undef it.
(Assignee)

Comment 1

2 years ago
I have run into the same problem while working on scroll-driven animations.

There, we have a WebIDL enum where one of the enumerators is "none" [1], which generates a C++ enum with a corresponding enumerator named |None| (whose name is not negotiable, since it's generated). I'd like to pass a variable of this enumeration type over the Layers API, but many files that include the relevant Layers API headers also include X11 headers.

I have a patch that at least partially implements a solution along the lines of what you describe.

[1] http://searchfox.org/mozilla-central/rev/0dd403224a5acb0702bdbf7ff405067f5d29c239/dom/webidl/AnimationEffectReadOnly.webidl#14
Blocks: 1281348
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8786087 - Flags: review?(jmuizelaar)

Comment 4

2 years ago
mozreview-review
Comment on attachment 8786087 [details]
Bug 1288686 - Avoid X11's |#define None 0L| intruding on other parts of the code.

https://reviewboard.mozilla.org/r/75058/#review73186

This seems fine. Add a comment to the RevertToNone part or remove it.

::: gfx/src/X11UndefineNone.h:18
(Diff revision 1)
> +// macro named "X11None".
> +// Include this header after including X11 headers, where necessary.
> +#ifdef None
> +#  undef None
> +#  define X11None 0L
> +#  ifdef RevertToNone

What's this part here for?
Attachment #8786087 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 5

2 years ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> ::: gfx/src/X11UndefineNone.h:18
> (Diff revision 1)
> > +// macro named "X11None".
> > +// Include this header after including X11 headers, where necessary.
> > +#ifdef None
> > +#  undef None
> > +#  define X11None 0L
> > +#  ifdef RevertToNone
> 
> What's this part here for?

X11/X.h defines a macro RevertToNone as (int)None, and we use that macro. Since I'm undefining None, that macro stops working, so I'm re-defining it to use X11None instead of None.
(Assignee)

Comment 6

2 years ago
(I'll add a comment as you suggested.)
(Assignee)

Updated

2 years ago
Assignee: nobody → botond
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 years ago
Updated to add comment.

Comment 9

2 years ago
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e760ad8231d6
Avoid X11's |#define None 0L| intruding on other parts of the code. r=jrmuizel
Blocks: 1299675

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e760ad8231d6
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Comment 11

2 years ago
Manish, did the change that landed here address your original issue?
Flags: needinfo?(manishearth)
(Reporter)

Comment 12

2 years ago
It looks like it should. One way to check would be to replace the None_s in nsStyleConsts.h with None, and update all the StyleFoo::None_ scattered around the codebase. I might do this later.
Flags: needinfo?(manishearth)
No longer blocks: 1325628

Updated

3 months ago
See Also: → bug 834505
You need to log in before you can comment on or make changes to this bug.