[Clang][Win] NotificationData.h(47,5): error: reference to 'UUID' is ambiguous

[Fujii Hironori]

After r287412 (Bug 234571), Clang-cl can't compile WinCairo port.

FAILED: Source/WebCore/CMakeFiles/WebCore.dir/__/__/WebCore/DerivedSources/unified-sources/UnifiedSource-cbdfe323-28.cpp.obj 
C:\PROGRA~1\LLVM\bin\clang-cl.exe (..)UnifiedSource-cbdfe323-28.cpp
In file included from WebCore\DerivedSources\unified-sources\UnifiedSource-cbdfe323-28.cpp:6:
In file included from ..\..\Source\WebCore\Modules/notifications/NotificationData.cpp:27:
..\..\Source\WebCore\Modules\notifications/NotificationData.h(47,5): error: reference to 'UUID' is ambiguous
    UUID notificationID;
    ^
WTF\Headers\wtf/UUID.h(143,12): note: candidate found by name lookup is 'UUID'
using WTF::UUID;
           ^
C:\Program Files (x86)\Windows Kits\10\include\10.0.17763.0\shared\rpcdce.h(83,14): note: candidate found by name lookup is 'UUID'
typedef GUID UUID;
             ^
1 error generated.

[Fujii Hironori]

Created attachment 447994 [details]
repro

[Fujii Hironori]

GCC and MSVC can compile, but Clang can't
https://godbolt.org/z/hfsd5Gxj1

https://stackoverflow.com/q/36204506
https://stackoverflow.com/q/55896837

[Radar WebKit Bug Importer]

<rdar://problem/87052994>

[Brady Eidson]

We shouldn't have to make WebKit source code worse to handle one misbehaving compiler.

I'd wager we've had to deal with this type of thing in the past - Anybody have any ideas?

[Fujii Hironori]

Created attachment 455038 [details]
workaround patch

[Darin Adler]

Comment on attachment 455038 [details]
workaround patch

I don’t think this is the best workaround.

[Fujii Hironori]

I don't intend to land this patch. Sometimes I have to compile Windows port with clang-cl. I need this small workaround patch in such a case.
The best solution is changing WebKit policy to avoid defining names in the global scope.

[Darin Adler]

OK, but we don’t have consensus on that.


Outside WTF, WebKit coding style absolutely does *not* support adding any identifiers to the global scope.

WTF has an approach where it uses a namespace to avoid link-time conflicts, but it *does* pull identifiers into global scope. Changing that WTF approach would be a *big* deal, and is not going to be decided based on a single identifier or bug. So if you’d like us to move in that direction, it would be good to start the discussion.

As I said we should not try to settle that here, but my first thought is that I think that since we used a short namespace name, WTF, we *could* consider making that change, but it would have far-reaching implications in code style. Writing WTF::String every time instead of just String. But maybe we could get some of the same benefit from importing these names into each of the WebKit namespaces like you did here?

[Fujii Hironori]

I'm proposing removing MSVC support by switch to clang-cl for WinCairo developers. But, some developers disagree.
So, this problem isn't so important now.
I think the simplest solution might be adding "using namespace WTF;" in all other namespaces.

[Fujii Hironori]

(In reply to Fujii Hironori from comment #9)
> So, this problem isn't so important now.

I'm going to revive clang-cl builds for Windows port.
This problem is important now.

> I think the simplest solution might be adding "using namespace WTF;" in all
> other namespaces.

This approach doesn't work. WTF::HashTable and JSC::HashTable conflict.
Source/JavaScriptCore/runtime/Lookup.h

[Fujii Hironori]

I misunderstood. "using namespace WTF;" in each namespace can't solve this problem.

[Fujii Hironori]

Pull request: https://github.com/WebKit/WebKit/pull/12784

[EWS]

Committed 263042@main (16e36e6bc5e6): <https://commits.webkit.org/263042@main>

Reviewed commits have been landed. Closing PR #12784 and removing active labels.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 255567

[Yusuke Suzuki]

Given that this broke internal build, probably we should stop using `using WTF::UUID`, and always use `WTF::UUID` for this class. Or rename it to some other name.

[Fujii Hironori]

Created attachment 465955 [details]
Patch to replace UUID with WTF::UUID

I created a patch to replace UUID with WTF::UUID, but check-webkit-style complains for this patch.
> ERROR: Source/WebKit/Shared/Notifications/NotificationManagerMessageHandler.messages.in:26:  Line contains WTF:: prefix.  [build/messagesin/wtf] [5]

I'm going to create another patch with the first approach and "#if OS(WINDOWS)".

[Fujii Hironori]

Pull request: https://github.com/WebKit/WebKit/pull/12843

[Fujii Hironori]

Pull request: https://github.com/WebKit/WebKit/pull/15553

[EWS]

Committed 265828@main (d8d3aef8dff8): <https://commits.webkit.org/265828@main>

Reviewed commits have been landed. Closing PR #15553 and removing active labels.