Closed Bug 1288686 Opened 4 years ago Closed 4 years ago
#define None in X11 causing problems
58 bytes, text/x-review-board-request
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" , 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.  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.)
Updated to add comment.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/e760ad8231d6 Avoid X11's |#define None 0L| intruding on other parts of the code. r=jrmuizel
Manish, did the change that landed here address your original issue?
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.
You need to log in before you can comment on or make changes to this bug.