The file should probably be marked @no-unify.
Created attachment 435989 [details] patch
<rdar://problem/82439830>
Created attachment 442489 [details] patch updated patch
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
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.
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?
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
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?
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
Then, we no longer need the #undef. could you remove all #undef for the issue in the patch?
you mean for the ANGLE bits?
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.
(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)
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.
(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
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.