Bug 229337 – GLContextGLX.cpp may sometimes fail build because of X11 include files

• Component: WebCore Misc.
• Status: NEW

[Nina Kolesa]

The file should probably be marked @no-unify.

[Nina Kolesa]

Created attachment 435989 [details]
patch

[Radar WebKit Bug Importer]

<rdar://problem/82439830>

[Nina Kolesa]

Created attachment 442489 [details]
patch

updated patch

[Fujii Hironori]

WebKit has a lot of #undef in WTF and WebCore layer for this issue, for example "#undef True" and "#undef None".
Does this change make it possible to remove it?
For example, https://github.com/WebKit/WebKit/blob/bb27304754d0bb686fc3ddfccc42fb8ec1f14d62/Source/WTF/wtf/Threading.h#L64,L68

[Fujii Hironori]

Maybe, No. Because it can't compile if both X11 header and undef-ing headers are included.
I think we should take #undef-ing approach.

[Fujii Hironori]

WebKit has ANGLEHeaders.h to include ANGLE headers.
https://github.com/WebKit/WebKit/blob/main/Source/WebCore/platform/graphics/angle/ANGLEHeaders.h

I think we should add X11Headers.h to include X11 headers.
WDYT?

[Nina Kolesa]

possibly, I marked the file no-unify since in practice other OpenGL files do the same thing (there's a bunch of others in the same file) and I think it's the cleanest way to get around it (it should work unconditionally regardless of what the X11 headers do)

but I'm fine either way

[Fujii Hironori]

In my understanding, @no-unify approach can't solve the issue completely because #include order is ruled by WebKit coding style.
If a header defining None or True are included after X11 header, it can't compile.
Is my understanding right?

[Nina Kolesa]

if the build for the file is non-unified, it solves the issue because the file is compiled on its own - and therefore we get control over *which* headers are included and can make sure they are non-conflicting

[Fujii Hironori]

Then, we no longer need the #undef. could you remove all #undef for the issue in the patch?

[Nina Kolesa]

you mean for the ANGLE bits?

[Fujii Hironori]

WebKit has a lot of #undef in WTF and WebCore layer for this issue
For example, https://github.com/WebKit/WebKit/blob/bb27304754d0bb686fc3ddfccc42fb8ec1f14d62/Source/WTF/wtf/Threading.h#L64,L68
If you can solve the issue completely by @no-unify approach, we no longer need the #undef.

I think @no-unify approach can't solve the issue completely  because #include order is ruled by WebKit coding style.
If a header defining None or True are included after X11 header, it can't compile. We need #undef. @no-unify approach isn't a good idea.
I think we should add X11Headers.h to include all X11 headers we need.

[Nina Kolesa]

(In reply to Fujii Hironori from comment #12)
> WebKit has a lot of #undef in WTF and WebCore layer for this issue
> For example,
> https://github.com/WebKit/WebKit/blob/
> bb27304754d0bb686fc3ddfccc42fb8ec1f14d62/Source/WTF/wtf/Threading.h#L64,L68
> If you can solve the issue completely by @no-unify approach, we no longer
> need the #undef.
> 
> I think @no-unify approach can't solve the issue completely  because
> #include order is ruled by WebKit coding style.
> If a header defining None or True are included after X11 header, it can't
> compile. We need #undef. @no-unify approach isn't a good idea.
> I think we should add X11Headers.h to include all X11 headers we need.

this is never really a problem for the problematic file in this case, because the X11 headers are included last, and it's a source file - which was my point all along, if you eliminate it being unified with another source file, there should be no problem anymore (your concern would only be a problem if the file itself was a header, or if we didn't control the include order of headers)

[Fujii Hironori]

Sonny, but I don't understand.
Can we remove all #undef?
For example, https://github.com/WebKit/WebKit/blob/bb27304754d0bb686fc3ddfccc42fb8ec1f14d62/Source/WTF/wtf/Threading.h#L64,L68
If so, please do it.
If not, please give up the @no-unify approach, that don't solve the issue.

[Nina Kolesa]

(In reply to Fujii Hironori from comment #14)
> Sonny, but I don't understand.
> Can we remove all #undef?
> For example,
> https://github.com/WebKit/WebKit/blob/
> bb27304754d0bb686fc3ddfccc42fb8ec1f14d62/Source/WTF/wtf/Threading.h#L64,L68
> If so, please do it.
> If not, please give up the @no-unify approach, that don't solve the issue.

it does not fix that because that's a header; this patch is specifically for the one failing file

[Michael Catanzaro]

I agree with Fujii that #undef is preferable to @no-unify.

Don't hesitate to use @no-unify if you really need to, but I don't see why #undef couldn't do the job here.