Created attachment 380730 [details] Build log Hi all, WebKit failed with lots of "error C3861: 'CFAutorelease': identifier not found" under /permissive- mode when built by msvc on Windows, I use latest source afa5598 on master branch. Could you help take a look at this? Repro steps: 1. git clone https://github.com/WebKit/webkit d:\WebKit\src 2. open a VS 2017 x86 command prompt as admin and browse to D:\Webkit 3. extra WebKitLibrary\win to D:\WebKit\src\WebKitLibraries\win 4. set path=C:\gnuwin32\bin;%path% 5. set _CL_=/permissive- 6. cmake -G "Visual Studio 15 2017" -DCMAKE_SYSTEM_VERSION=10.0.17134.0 -DCMAKE_BUILD_TYPE=Release -DRUBY_LIBRARY=C:\Ruby25-x64\lib -DPORT="AppleWin" ..\src 7. msbuild /p:Configuration=Release;Platform=Win32 WebKit.sln /t:Rebuild Failures(detail see attachment): DateMath.cpp D:\WebKit\src\Source\WTF\wtf/RetainPtr.h(196): error C3861: 'CFAutorelease': identifier not found D:\WebKit\src\Source\WTF\wtf/RetainPtr.h(196): note: 'CFAutorelease': function declaration must be available as none of the arguments depend on a template parameter
Created attachment 380731 [details] Patch
Created attachment 380824 [details] webkit patch new error was generated after applying this webkit.patch file
Created attachment 380825 [details] webkit_result_log The webkit result log was generated after applying patch.(please see attached patch file)
Created attachment 447302 [details] Patch
Comment on attachment 447302 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=447302&action=review > Source/WTF/ChangeLog:10 > + * wtf/RetainPtr.h: Enclosed RetainPtr<T>::autorelease with #if !OS(WINDOWS). This patch wraps the entire RetainPtr class in #if !OS(WINDOWS), but that’s not what the comment says.
Comment on attachment 447302 [details] Patch I think it’s OK to put a condition in here around the parts that won’t work on Windows, but: 1) That’s not what this patch does (and so that’s why review-). 2) I don’t think OS(WINDOWS) is the best conditional to use. I think we should use HAVE(CFAUTORELEASE) or HAVE(AUTORELEASE) even though we need to add to PlatformHave.h to do that.
Comment on attachment 447302 [details] Patch My bad. this is a wrong patch.
(In reply to Darin Adler from comment #6) > 2) I don’t think OS(WINDOWS) is the best conditional to use. I think we > should use HAVE(CFAUTORELEASE) or HAVE(AUTORELEASE) even though we need to > add to PlatformHave.h to do that. I checked AppleWin's latest WebKitSupportLibrary.zip and WebKitAuxiliaryLibrary.zip. They don't have CFAutorelease at least for now. I think OS(WINDOWS) is a good condition at least as now now. If AppleWin will add it in the future version, it will be a good time to add the new macro.
Created attachment 447430 [details] Patch
(In reply to Fujii Hironori from comment #8) > I think OS(WINDOWS) is a good condition at least as now now. I think it’s "hacky" to put just OS(WINDOWS). Over time, decisions like this make it hard to code correctly on the project. I feel the same way about overuse of PLATFORM(COCOA), PLATFORM(MAC), etc. But even more, "OS" is not supposed to be used to understand the availability of higher level frameworks. This is a misuse.
I suggest: #if PLATFORM(COCOA) instead of #if !OS(WINDOWS) I still don’t think it’s great, but I think it’s better.
And you should probably do the same for the declaration: PtrType autorelease();
That’s also OK, but not required. I guess it could save a programmer some time since compile will fail rather than link.
Created attachment 447463 [details] Patch
(In reply to Darin Adler from comment #6) > Comment on attachment 447302 [details] > Patch > > I think it’s OK to put a condition in here around the parts that won’t work > on Windows, but: > > 1) That’s not what this patch does (and so that’s why review-). > 2) I don’t think OS(WINDOWS) is the best conditional to use. I think we > should use HAVE(CFAUTORELEASE) or HAVE(AUTORELEASE) even though we need to > add to PlatformHave.h to do that. You should be able to do a check for this in the `OptionsWin.cmake` file. There's a macro for seeing if a symbol is present in the CMake code. I think doing something along the lines of this should work if (USE_CF) set(CMAKE_REQUIRED_INCLUDES ${WEBKIT_LIBRARIES_INCLUDE_DIR}) set(CMAKE_REQUIRED_LIBRARIES "${WEBKIT_LIBRARIES_LINK_DIR}/CoreFoundation${DEBUG_SUFFIX}.lib") WEBKIT_CHECK_HAVE_SYMBOLE(HAVE_AUTORELEASE CFAutorelease CoreFoundation/CoreFoundation.h) endif () See https://github.com/WebKit/WebKit/blob/main/Source/cmake/OptionsAppleWin.cmake#L50-L94
Committed r287200 (245367@main): <https://commits.webkit.org/245367@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 447463 [details].
In the shorter term, if we want to use HAVE(CFAUTORELEASE) in the short term instead of PLATFORM, the main thing we would need to do is not at all Windows-specific; we’d add this to PlatforHave.h: #if PLATFORM(COCOA) #define HAVE_CFAUTORELEASE 1 #endif (In reply to Don Olmstead from comment #15) > You should be able to do a check for this in the `OptionsWin.cmake` file. > There's a macro for seeing if a symbol is present in the CMake code. That would be a nice refinement some day if we ever do have a Windows-platform variant that has CFAutorelease, but I think that’s not likely, so this would be of theoretical value, but not practical value. Or if we ever reach the point where we use CMake on all Cocoa platforms, then cross platform we could consider adding this to some platform-independent CMake source file: if (USE_CF) WEBKIT_CHECK_HAVE_SYMBOL(HAVE_AUTORELEASE CFAutorelease CoreFoundation/CoreFoundation.h) endif ()
Thank you for the feedback. I reopen this ticket to revise.
Created attachment 447570 [details] Patch
Committed r287265 (245421@main): <https://commits.webkit.org/245421@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 447570 [details].