Skip to content

Use __has_declspec_attribute for shared builds#3616

Closed
donny-dont wants to merge 1 commit intocurl:masterfrom
donny-dont:declspec
Closed

Use __has_declspec_attribute for shared builds#3616
donny-dont wants to merge 1 commit intocurl:masterfrom
donny-dont:declspec

Conversation

@donny-dont
Copy link
Contributor

Clang compilation targets may support __declspec attributes. Because of that it has a __has_declspec_attribute check.

Currently cURL just assumes that __declspec is Windows only. This adds a compatibility macro and checks whether dllimport and dllexport are available.

@bagder
Copy link
Member

bagder commented Feb 25, 2019

Okay, but do we really want this for non-windows?

@donny-dont
Copy link
Contributor Author

Any toolchain that's clang based could potentially use __declspec. We use it on the PlayStation and ship cURL

@bagder
Copy link
Member

bagder commented Feb 26, 2019

Right, they can use it but with this change they will use it - unconditionally. I'm not even sure what the significance is of using this, but it is certainly a change to how it worked without this change.

BTW, the added line is too long which why the CI turns red:

../../include/curl/curl.h:117:146: warning: Longer than 79 columns (LONGLINE)

@donny-dont
Copy link
Contributor Author

When you build clang out for a platform you have to explicitly turn on support for __declspec so this would only affect a platform that wants to use it.

I figured I'd see if the change was palatable to upstream. If its not I'm fine with maintaining it outside of curl.

@bagder
Copy link
Member

bagder commented Feb 27, 2019

this would only affect a platform that wants to use it.

Ah, right. Thanks for explaining this for an ignorant like me! 😁 I'll merge...

@bagder bagder closed this in 50482b8 Feb 27, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants