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

DYLIB_API questionable #36

Closed
eyalroz opened this issue May 31, 2022 · 3 comments · Fixed by #42
Closed

DYLIB_API questionable #36

eyalroz opened this issue May 31, 2022 · 3 comments · Fixed by #42
Assignees
Labels
enhancement New feature or request

Comments

@eyalroz
Copy link
Contributor

eyalroz commented May 31, 2022

dylib is not supposed to have any requirements from dynamic-linking libraries. They exist entirely independently of it, ignorant of its existence. So, where would they know about DYLIB_API from? ... nowhere.

As for the code linking against them, using dylib - it shouldn't have to know about that either; it just defines a function type, e.g.int(void), and get_function()'s with that.

So - it seems to me like this:

#if defined(_WIN32) || defined(_WIN64)
#define DYLIB_API extern "C" __declspec(dllexport)
#else
#define DYLIB_API extern "C"
#endif

is unnecessary. Perhaps it belongs in the test library?

@martin-olivier
Copy link
Owner

I'm currently working on #27 to be able to load C++ mangled symbols, and as you said DYLIB_API will become quite useless.

Here is the current changes on this on my local work :

#if defined(_WIN32) || defined(_WIN64)
#define DYLIB_EXPORT __declspec(dllexport)
#else
#define DYLIB_EXPORT
#endif

The user will be able to totally get rid of DYLIB_EXPORT by using the following cmake rule :
(I will add this in the documentation)

if(MSVC)
    set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS TRUE)
endif()

@martin-olivier martin-olivier self-assigned this Jun 1, 2022
@martin-olivier martin-olivier added the enhancement New feature or request label Jun 1, 2022
@eyalroz
Copy link
Contributor Author

eyalroz commented Jun 1, 2022

I'm saying that the user can't use DYLIB_API anyway, already today. The user is not the person writing the dynamic library...

@martin-olivier
Copy link
Owner

martin-olivier commented Jun 1, 2022

Mhhh yes I see your point.
Originally, I made this library to facilitate my school projects.
Since we were providing both core and dlls, I used it to export/unmangle my factory functions.

But yes, dlls shouldn't include dylib just for that macro and in most cases dlls are shipped by other people than the one writing the core.

I will remove DYLIB_API / DYLIB_EXPORT

@martin-olivier martin-olivier linked a pull request Jun 3, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants