Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use __has_declspec_attribute for shared builds #3616

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
2 participants
@donny-dont
Copy link
Contributor

commented Feb 25, 2019

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

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

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

@donny-dont

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

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

@bagder

This comment has been minimized.

Copy link
Member

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 donny-dont force-pushed the donny-dont:declspec branch from e4a573c to 505edb0 Feb 26, 2019

@donny-dont

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

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

This comment has been minimized.

Copy link
Member

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.
You can’t perform that action at this time.