Bug 182479

Created attachment 333074 [details] Patch

>ERROR: '/Volumes/Data/EWS/WebKit/WebKitBuild/Release/WebKitFailed to run "['Tools/Scripts/build-webkit', '--release']" exit_code: 65 >Last 500 characters of output: >A55DEAA61670402E003DB841.sh >ERROR: >'/Volumes/Data/EWS/WebKit/WebKitBuild/Release/WebKit.framework/Versions/A/PrivateHeaders/WKRetainPtr.h:245' included forbidden macro 'COMPILER' => '#if !COMPILER(MSVC)' >Command /bin/sh failed with exit code 1 > >** BUILD FAILED ** > >The following build commands failed: > PhaseScriptExecution Check\ For\ Inappropriate\ Macros\ in\ External\ Headers /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Script-A55DEAA61670402E003DB841.sh >(1 failure) Mac and iOS failed by the same reason like the above.

Comment on attachment 333074 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333074&action=review > Source/WebKit/UIProcess/API/cpp/WKRetainPtr.h:245 > +#if !COMPILER(MSVC) We usually try to put the positive first, so this would be #if COMPILER(MSVC) But you can't use COMPILER here because this header is part of the API. Elsewhere in the API we've done #if defined(WIN32) || defined(_WIN32), but maybe we should check _MSC_VER instead for people using clang on windows.

(In reply to Alex Christensen from comment #3) > We usually try to put the positive first, so this would be #if COMPILER(MSVC) > But you can't use COMPILER here because this header is part of the API. > Elsewhere in the API we've done #if defined(WIN32) || defined(_WIN32), but > maybe we should check _MSC_VER instead for people using clang on windows. Thank you for your advice. I'll fix it with the condition you suggested.

Created attachment 333152 [details] Patch

(In reply to Alex Christensen from comment #3) > maybe we should check _MSC_VER instead for people using clang on windows. Table about _MSC_VER and Visual Studio version: https://blogs.msdn.microsoft.com/vcblog/2015/12/04/clang-with-microsoft-codegen-in-vs-2015-update-1/ https://cpprefjp.github.io/implementation.html#visual_cpp I have two questions about _MSC_VER. 1. Is VisualStudio 2017 a recommended IDE on windows ? 2. When clang is used on windows, which template should be chosen, a) or b)? a) template<typename T> inline WKRetainPtr<T> adoptWK(T) _Check_return_; b) template<typename T> inline WKRetainPtr<T> adoptWK(T) __attribute__((warn_unused_result));

Comment on attachment 333152 [details] Patch I confirmed this patch works on the wincairo build but the Bot status was failure on wincairo. If we take care of clang users on windows, for example, the part will be as follows. (In this case, supposing VisualStudio 2017 and Clang/C2 are used.) Please give me your comment. #if defined(WIN32) || defined(_WIN32) #if (_MSC_VER > 1900) && (__c2__) template<typename T> inline WKRetainPtr<T> adoptWK(T) __attribute__((warn_unused_result)); #else template<typename T> inline WKRetainPtr<T> adoptWK(T) _Check_return_; #endif #else template<typename T> inline WKRetainPtr<T> adoptWK(T) __attribute__((warn_unused_result)); #endif

Comment on attachment 333152 [details] Patch This is the wrong fix. The correct fix would be to use WARN_UNUSED_RETURN from Compiler.h instead of using warn_unused_result directly.

Comment on attachment 333152 [details] Patch Oh wait, I see, this is in an "API" header. Maybe this is OK, then. Lets try it at least.

Comment on attachment 333152 [details] Patch Actually, the compiler version issue you mentioned has to be resolved first, so back to review-.

Created attachment 333892 [details] Patch

Created attachment 333909 [details] Patch

(In reply to Darin Adler from comment #10) > Actually, the compiler version issue you mentioned has to be resolved first, > so back to review-. > Created attachment 333909 [details] This patch fixed the compiler version issue. Could you review it again?

Comment on attachment 333909 [details] Patch Rejecting attachment 333909 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 333909, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: rdparty/autoinstalled/mechanize/_urllib2_fork.py", line 332, in _call_chain result = func(*args) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open return self.do_open(conn_factory, req) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open raise URLError(err) urllib2.URLError: <urlopen error [Errno 60] Operation timed out> Full output: http://webkit-queues.webkit.org/results/6563066

Created attachment 334223 [details] Patch (a rebased version) This patch is a rebased version of attachment 333909 [details].

Created attachment 334224 [details] Patch (a rebased version)

Comment on attachment 334224 [details] Patch (a rebased version) Clearing flags on attachment: 334224 Committed r228751: <https://trac.webkit.org/changeset/228751>

Not sure why this didn't get closed automagically.

<rdar://problem/37715172>