Bug 182479 – [Win] Fix MSVC's treating __attribute__((warn_unused_result))

• Component: WebKit2
• Status: RESOLVED FIXED

[Yousuke Kimoto]

MSVC doesn't understand __attribute((warn_unsed_resutl)), but it has a similar option, _Check_return_.

MSVC Annotating Function Behavior:
https://msdn.microsoft.com/en-us/library/jj159529.aspx

[Yousuke Kimoto]

Created attachment 333074 [details]
Patch

[Yousuke Kimoto]

>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.

[Alex Christensen]

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.

[Yousuke Kimoto]

(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.

[Yousuke Kimoto]

Created attachment 333152 [details]
Patch

[Yousuke Kimoto]

(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));

[Yousuke Kimoto]

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

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

Comment on attachment 333152 [details]
Patch

Actually, the compiler version issue you mentioned has to be resolved first, so back to review-.

[Yousuke Kimoto]

Created attachment 333892 [details]
Patch

[Yousuke Kimoto]

Created attachment 333909 [details]
Patch

[Yousuke Kimoto]

(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?

[WebKit Commit Bot]

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

[Yousuke Kimoto]

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

This patch is a rebased version of attachment 333909 [details].

[Yousuke Kimoto]

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

[WebKit Commit Bot]

Comment on attachment 334224 [details]
Patch (a rebased version)

Clearing flags on attachment: 334224

Committed r228751: <https://trac.webkit.org/changeset/228751>

[Don Olmstead]

Not sure why this didn't get closed automagically.

[Radar WebKit Bug Importer]

<rdar://problem/37715172>