Closed Bug 1288686 Opened 4 years ago Closed 4 years ago

#define None in X11 causing problems

Categories

(Core :: General, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: manishearth, Assigned: botond)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
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
Attachment #8786087 - Flags: review?(jmuizelaar)
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+
(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.
(I'll add a comment as you suggested.)
Assignee: nobody → botond
Updated to add comment.
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
https://hg.mozilla.org/mozilla-central/rev/e760ad8231d6
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Manish, did the change that landed here address your original issue?
Flags: needinfo?(manishearth)
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)
Blocks: 1325628
No longer blocks: 1325628
See Also: → 834505
You need to log in before you can comment on or make changes to this bug.