Bug 179893 – Add declspec within WebKit API

• Component: WebKit API
• Status: RESOLVED FIXED

[Don Olmstead]

Add __declspec to WebKit API for Windows.

[Don Olmstead]

Created attachment 327371 [details]
Patch

Adds declspec export/import along with a check for clang support of it.

[Darin Adler]

Comment on attachment 327371 [details]
Patch

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

> Source/WebKit/Shared/API/c/WKDeclarationSpecifiers.h:33
> +#if !defined(__has_declspec_attribute)
> +#define __has_declspec_attribute(x) 0
> +#endif

I suggest putting this in a separate paragraph, not inside the WK_EXPORT paragraph. As is done with __has_extension and __has_attribute below.

Also, should use ifndef as all the others do.

> Source/WebKit/Shared/API/c/WKDeclarationSpecifiers.h:41
> +#endif /* BUILDING_WEBKIT */

Regardless of the overall merits of this patch, it’d not good to have a comment here that does not match the #if above. The thing about is BUILDING_WebKit, but the comment says BUILDING_WEBKIT.

[Don Olmstead]

Created attachment 327377 [details]
Patch

Updating based on review comments. Adding RealView ARM Compiler support of declspec.

[Darin Adler]

Comment on attachment 327377 [details]
Patch

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

Seems OK to land it like this, although it doesn’t seem exactly right.

> Source/WebKit/Shared/API/c/WKDeclarationSpecifiers.h:36
> +#ifndef __has_declspec_attribute
> +#if defined(WIN32) || defined(_WIN32) || defined(__CC_ARM) || defined(__ARMCC__)
> +#define __has_declspec_attribute(x) 1
> +#else
> +#define __has_declspec_attribute(x) 0
> +#endif
> +#endif

This doesn’t seem quite right to me; it’s not great that __has_declspec_attribute(THIS_DECLSPEC_ATTRIBUTE_IS_BOGUS_AND_DOES_NOT_EXIST) will return true on many platforms.

[Don Olmstead]

(In reply to Darin Adler from comment #4)
> Comment on attachment 327377 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327377&action=review
> 
> Seems OK to land it like this, although it doesn’t seem exactly right.
> 
> > Source/WebKit/Shared/API/c/WKDeclarationSpecifiers.h:36
> > +#ifndef __has_declspec_attribute
> > +#if defined(WIN32) || defined(_WIN32) || defined(__CC_ARM) || defined(__ARMCC__)
> > +#define __has_declspec_attribute(x) 1
> > +#else
> > +#define __has_declspec_attribute(x) 0
> > +#endif
> > +#endif
> 
> This doesn’t seem quite right to me; it’s not great that
> __has_declspec_attribute(THIS_DECLSPEC_ATTRIBUTE_IS_BOGUS_AND_DOES_NOT_EXIST)
> will return true on many platforms.

Would you prefer it to do

#ifndef __has_declspec_attribute
#define __has_declspec_attribute(x) 0
#endif

then use it like

#elif defined(WIN32) || defined(_WIN32) || defined(__CC_ARM) || defined(__ARMCC__) || (__has_declspec_attribute(dllimport) && __has_declspec_attribute(dllexport))

[Darin Adler]

(In reply to Don Olmstead from comment #5)
> Would you prefer it to do
> 
> #ifndef __has_declspec_attribute
> #define __has_declspec_attribute(x) 0
> #endif
> 
> then use it like
> 
> #elif defined(WIN32) || defined(_WIN32) || defined(__CC_ARM) ||
> defined(__ARMCC__) || (__has_declspec_attribute(dllimport) &&
> __has_declspec_attribute(dllexport))

I do think that’s slightly better.

[Don Olmstead]

Created attachment 327379 [details]
Patch

Addressing feedback

[WebKit Commit Bot]

Comment on attachment 327379 [details]
Patch

Clearing flags on attachment: 327379

Committed r225064: <https://trac.webkit.org/changeset/225064>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35651307>