btzy / nativefiledialog-extended

Cross platform (Windows, Mac, Linux) native file dialog library with C and C++ bindings, based on mlabbe/nativefiledialog.
zlib License
663 stars 94 forks source link

Add support for building shared libraries on Windows #109

Closed tomix1024 closed 1 year ago

tomix1024 commented 1 year ago

If CMake is configured to create a shared library (.dll) for this library, the symbols (function names) are not exported and an empty dll file is created. On Windows, the compiler has to be invited explicitly by the programmer to export (or import) a function from a shared library (e.g. using __declspec(dllexport) attributes... It is common to define a LIBRARY_NAME_API macro for this purpose and prefix it to any publicly visible function declaration.

This PR adds support for this convention.

btzy commented 1 year ago

Thanks! This seems correct on Windows, but do you know why the CI build for Windows (MSVC, Shared) has been passing even without __declspec(dllexport)?

Also, could you elaborate on why __attribute__((visibility("default"))) is required on Mac and Linux? It appears that visibility("default") is, in fact, the default.

tomix1024 commented 1 year ago

do you know why the CI build for Windows (MSVC, Shared) has been passing even without __declspec(dllexport)?

Yes, the CI script passed the argument -DCMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=${{ matrix.shared_lib.flag }} to CMake, which tells CMake to configure the compiler in such a way that all symbols are always exported, irrespective of the declspec(...). In the last commit I removed that argument, and CI should still pass on this branch.

could you elaborate on why attribute((visibility("default"))) is required on Mac and Linux? It appears that visibility("default") is, in fact, the default.

I also think that it is the default, so why set it explicitly? It is possible to change the default behaviour by passing -fvisibility=h to the compiler, for example. https://gcc.gnu.org/wiki/Visibility So even though visibility("default") is usually the default, it could change, and therefore we are on the safe side being explicit about the visibility of the API functions.

btzy commented 1 year ago

Thanks for the elaboration, this seems good to me!