Bug 171240 - [Win] Use Clang's __has_declspec_attribute for export macros
Summary: [Win] Use Clang's __has_declspec_attribute for export macros
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-24 12:07 PDT by Don Olmstead
Modified: 2017-04-25 14:46 PDT (History)
13 users (show)

See Also:


Attachments
Patch (4.58 KB, patch)
2017-04-24 12:10 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Don Olmstead 2017-04-24 12:07:47 PDT
Currently a OS(WINDOWS) check is used to determine whether __declspec should be used for exporting symbols in shared libraries. Clang defines a macro __has_declspec_attribute that can be used to determine if the platform supports __declspec.
Comment 1 Don Olmstead 2017-04-24 12:10:42 PDT
Created attachment 307995 [details]
Patch
Comment 2 Build Bot 2017-04-24 12:13:02 PDT
Attachment 307995 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Compiler.h:54:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WTF/wtf/Compiler.h:54:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Alex Christensen 2017-04-24 16:25:16 PDT
Comment on attachment 307995 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307995&action=review

> Source/WTF/wtf/Platform.h:440
> +#if OS(WINDOWS) \
> +    || (COMPILER_HAS_CLANG_DECLSPEC(dllimport) && COMPILER_HAS_CLANG_DECLSPEC(dllexport))

These can be on one line.
Also, if it has __declspec(dllimport) can't we assume it has __declspec(dllexport)?

> Source/WTF/wtf/Platform.h:441
> +#define USE_DECLSPEC_ATTRIBUTE 1

__declspec is a keyword according to https://msdn.microsoft.com/en-us/library/dabb5z75.aspx
I wish clang had used __has_declspec_keyword, but they didn't.

> Source/WTF/wtf/Platform.h:443
> +#elif defined(__GNUC__) && !defined(__CC_ARM) && !defined(__ARMCC__)

Is there no visibility attribute on ARM?
Comment 4 Don Olmstead 2017-04-25 11:01:15 PDT
(In reply to Alex Christensen from comment #3)
> Comment on attachment 307995 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=307995&action=review
> 
> > Source/WTF/wtf/Platform.h:440
> > +#if OS(WINDOWS) \
> > +    || (COMPILER_HAS_CLANG_DECLSPEC(dllimport) && COMPILER_HAS_CLANG_DECLSPEC(dllexport))
> 
> These can be on one line.
> Also, if it has __declspec(dllimport) can't we assume it has
> __declspec(dllexport)?

I would assume that would be the case but just made it completely explicit. Can change that.

> 
> > Source/WTF/wtf/Platform.h:441
> > +#define USE_DECLSPEC_ATTRIBUTE 1
> 
> __declspec is a keyword according to
> https://msdn.microsoft.com/en-us/library/dabb5z75.aspx
> I wish clang had used __has_declspec_keyword, but they didn't.
> 
> > Source/WTF/wtf/Platform.h:443
> > +#elif defined(__GNUC__) && !defined(__CC_ARM) && !defined(__ARMCC__)
> 
> Is there no visibility attribute on ARM?

That line has actually been there since the first commit of ExportMacros.h so its over 6 years old. I'm not sure the initial thinking of it. Should we just remove it?
Comment 5 Alex Christensen 2017-04-25 14:14:07 PDT
Comment on attachment 307995 [details]
Patch

Let's just do this as it is now.
Comment 6 WebKit Commit Bot 2017-04-25 14:46:57 PDT
Comment on attachment 307995 [details]
Patch

Clearing flags on attachment: 307995

Committed r215766: <http://trac.webkit.org/changeset/215766>
Comment 7 WebKit Commit Bot 2017-04-25 14:46:59 PDT
All reviewed patches have been landed.  Closing bug.