Summary: | [Win] Use Clang's __has_declspec_attribute for export macros | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Don Olmstead <don.olmstead> | ||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | achristensen, benjamin, buildbot, cdumez, cmarcelo, commit-queue, dbates, Hironori.Fujii, keith_miller, lforschler, mark.lam, msaboff, saam | ||||
Priority: | P2 | ||||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Don Olmstead
2017-04-24 12:07:47 PDT
Created attachment 307995 [details]
Patch
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 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? (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 on attachment 307995 [details]
Patch
Let's just do this as it is now.
Comment on attachment 307995 [details] Patch Clearing flags on attachment: 307995 Committed r215766: <http://trac.webkit.org/changeset/215766> All reviewed patches have been landed. Closing bug. |