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

Improve dll export/import definition #181

Merged
merged 1 commit into from
Jan 4, 2024

Conversation

bwrsandman
Copy link
Contributor

Follow-up to #178

  • Update API naming for clarity and consistency by renaming UNSHIELD_DLLEXPORT to UNSHIELD_API. This change enhances naming clarity, making the purpose of the macro more intuitive and aligned with common naming conventions in cross- platform libraries.

  • Optimize performance with export/import directives. By differentiating between __declspec(dllexport) and __declspec(dllimport) in Windows builds, this adjustment improves the performance of dynamic linking, ensuring efficient symbol resolution and potentially reducing binary size.

  • Enhance flexibility in CMake configuration. Applying PRIVATE and PUBLIC specifiers in target_compile_definitions ensures that UNSHIELD_EXPORT is used internally within the library, while UNSHIELD_DYNAMIC_LIBRARY is propagated to clients of the library. This distinction prevents macro leaks and provides users with the appropriate definitions based on their linking method, offering greater flexibility and ease of integration in diverse build configurations.

  • Substitute _MSC_VER with _WIN32 to broaden compatibility, ensuring that the library correctly handles DLL exports and imports across various Windows compilers, not just MSVC, thereby facilitating cross-platform builds. Indeed, __declspec(dllexport) and __declspec(dllimport) is a Windows convention instead of a Visual Studio one.

@bwrsandman
Copy link
Contributor Author

Would like a review/test from @elasota in case I missed or broke something.

@bwrsandman bwrsandman marked this pull request as draft January 3, 2024 18:20
* Update API naming for clarity and consistency by renaming
  `UNSHIELD_DLLEXPORT` to `UNSHIELD_API`. This change enhances naming
  clarity, making the purpose of the macro more intuitive and aligned with
  common naming conventions in cross-platform libraries.
* Optimize performance with export/import directives. By differentiating
  between `__declspec(dllexport)` and `__declspec(dllimport)` in Windows
  builds, this adjustment improves the performance of dynamic linking,
  ensuring efficient symbol resolution and potentially reducing binary
  size.
* Enhance flexibility in CMake configuration. Applying `PRIVATE` and
  `PUBLIC` specifiers in `target_compile_definitions` ensures that
  `UNSHIELD_EXPORT` is used internally within the library, while
  `UNSHIELD_DYNAMIC_LIBRARY` is propagated to clients of the library. This
  distinction prevents macro leaks and provides users with the appropriate
  definitions based on their linking method, offering greater flexibility
  and ease of integration in diverse build configurations.
* Substitute `_MSC_VER` with `_WIN32` to broaden compatibility, ensuring
  that the library correctly handles DLL exports and imports across various
  Windows compilers, not just MSVC, thereby facilitating cross-platform
  builds. Indeed, `__declspec(dllexport)` and `__declspec(dllimport)` is a
  Windows convention instead of a Visual Studio one.
@bwrsandman bwrsandman marked this pull request as ready for review January 3, 2024 18:24
@twogood
Copy link
Owner

twogood commented Jan 3, 2024

Mad CMake skills @bwrsandman !

@elasota
Copy link
Contributor

elasota commented Jan 4, 2024

Seems okay to me

@twogood twogood merged commit bd7a6be into twogood:main Jan 4, 2024
6 checks passed
@bwrsandman bwrsandman deleted the UNSHIELD_API branch January 4, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants