njh / twolame

MPEG Audio Layer 2 (MP2) encoder
http://www.twolame.org/
GNU Lesser General Public License v2.1
59 stars 34 forks source link

usage of DLL_EXPORT for symbol visibility is wrong #85

Closed vtorri closed 5 years ago

vtorri commented 5 years ago

after applying PR #84 the compilation fails on Windows because of wrong declaration specification (DLL_EXPORT in twolame.h)

first, there is no need to define LIBTWOLAME_STATIC or LIBTWOLAME_DLL_EXPORTS : when using libtool, you know if libtool builds a DLL or not with the macro DLL_EXPORT (defined in case libtool builds the DLL, not defined otherwise)

so to define exported symbols i would not define DLL_EXPORT as it's a macro defined by libtool

i would do the following (that is what we do in our project) :

ifdef _WIN32

ifdef TWOLAME_BUILD

ifdef DLL_EXPORT

define TL_API __declspec(dllexport)

else

define TL_API

endif

else

define TL_API __declspec(dllimport)

endif

else

ifdef GNUC

if GNUC >= 4

define TL_API attribute ((visibility("default")))

else

define TL_API

endif

else

define TL_API

endif

endif

-DTWOLAME_BUILD is passed to CPPFLAGS of the library in Makefile.am (used only on Windows, to specify that the DLL is built)

and i would use TL_API instead of DLL_EXPORT in front of all the exported symbols

this also the usage of visibility flag on unix.

after this change, everything compiles flawlessly

i can do a pull request if you want

vtorri commented 5 years ago

it would be nice to have that before the next release

i'm compiling ffmpeg/gstreamer from scratch (with the same toolchain and compiler) with a lot of their dependencies. twolame is one of them.

vtorri commented 5 years ago

do you want me to create a PR ?

njh commented 5 years ago

Fixed in e52373e545e3a6b6addba258223310a427046594