-
Notifications
You must be signed in to change notification settings - Fork 101
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
Added LM_STATIC preprocessor flag when doing a static build #243
base: master
Are you sure you want to change the base?
Conversation
include/libmem/libmem.h
Outdated
#ifdef LM_STATIC | ||
# define LM_API | ||
#else | ||
# define LM_API LM_API_IMPORT | ||
# ifdef LM_EXPORT | ||
# define LM_API LM_API_EXPORT | ||
# else | ||
# define LM_API LM_API_IMPORT | ||
# endif | ||
#endif |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I've just pushed an update.
d058f77
to
0aee016
Compare
Does it not work with LM_EXPORT defined in static builds? |
CMakeLists.txt
Outdated
else() | ||
add_library(libmem SHARED ${LIBMEM_SRC}) | ||
target_compile_definitions(libmem PUBLIC LM_EXPORT) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
afaik if defined & during MSVC windows build it will let you build LIB. if not defined it should let you build DLL on windows OS by MSVC toolchain. |
It does work with __declspec(dllexport) when building static libraries. Not even sure what the consequences are or if the methods get exported if the .lib is linked into a .dll. I think the compiler creates some trampoline methods when dllexport is specified though.
|
@@ -97,8 +97,10 @@ set(LIBMEM_DEPS | |||
|
|||
if (LIBMEM_BUILD_STATIC) | |||
add_library(libmem STATIC ${LIBMEM_SRC}) | |||
target_compile_definitions(libmem PUBLIC LM_STATIC) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
CMakeLists.txt
Outdated
else() | ||
add_library(libmem SHARED ${LIBMEM_SRC}) | ||
target_compile_definitions(libmem PUBLIC LM_EXPORT) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
CMakeLists.txt
Outdated
@@ -115,7 +117,6 @@ if (LIBMEM_BUILD_TESTS) | |||
endif() | |||
|
|||
set_target_properties(libmem PROPERTIES POSITION_INDEPENDENT_CODE True INCLUDES ${LIBMEM_INC}) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand these suggestions. Don't we want to add LM_STATIC only if "LIBMEM_BUILD_STATIC" is true?
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, the ideal would be, for any OS:
StaticBuild: LM_STATIC
DynamicBuild: LM_EXPORT
It's also fine if we have LM_EXPORT all the time, like so:
StaticBuild: LM_STATIC and LM_EXPORT
DynamicBuild: LM_EXPORT
But the changes you suggested make it like this:
StaticBuild: LM_STATIC and LM_EXPORT
DynamicBuild: LM_STATIC and LM_EXPORT
So that it never adds the "__declspec(dllexport)" and so a dynamic library is never produced.
Or am I misunderstanding the changes?
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
CMakeLists.txt
Outdated
@@ -115,7 +117,6 @@ if (LIBMEM_BUILD_TESTS) | |||
endif() | |||
|
|||
set_target_properties(libmem PROPERTIES POSITION_INDEPENDENT_CODE True INCLUDES ${LIBMEM_INC}) | |||
target_compile_definitions(libmem PUBLIC LM_EXPORT) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's fine, although unnecessary, to add LM_EXPORT during static builds since in the .h file, the "#ifdef LM_STATIC" comes first.
@@ -97,8 +97,10 @@ set(LIBMEM_DEPS | |||
|
|||
if (LIBMEM_BUILD_STATIC) | |||
add_library(libmem STATIC ${LIBMEM_SRC}) | |||
target_compile_definitions(libmem PUBLIC LM_STATIC) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just added LM_EXPORT to both static and shared library. Seems like a good change that keeps backwards compatibility.
Yes it works, this one is optimization targeted for Windows OS I guess. Dunno too problematic to push it without testing existing CI workflows that build libmem itself & Rust/Python3 bindings over different OS-es as well. Maybe bindings would need rework, but in case of OS-es it should be fine.
Do you tend to keep only one define for this situation such as LM_EXPORT or LM_STATIC (This one keeps stuff relying over EXPORT defininition https://github.com/libsdl-org/SDL/blob/d1af211010adb1030ce04860437fd5a580cf8eed/include/begin_code.h#L60)? Overall it looks good now. |
0aee016
to
bdfa20d
Compare
Simple PR to avoid adding "__declspec(dllexport)" to function declarations when creating a static library.