quixdb / squash

Compression abstraction library and utilities
https://quixdb.github.io/squash/
MIT License
407 stars 53 forks source link

Add (and use) macro for internal symbols #177

Closed nemequ closed 8 years ago

nemequ commented 8 years ago

Right now we basically depend on -fvisibility=hidden being set in order to restrict symbol visibility by default. This is fine for GCC (and clang, and icc, etc.), but I'm pretty sure MSVC doesn't know about it. It would probably be wise to add a SQUASH_INTERNAL macro (defined to __attribute__ ((visibility ("hidden")))/__declspec(dllimport), and use that for all our non-static internal functions to make sure internals symbols are hidden more reliably.

That said, we can't really completely replace -fvisibility=hidden since we're unlikely to be able to modify all plugins to explicitly specify visibility.

jibsen commented 8 years ago

MSVC leaves all symbols hidden by default, and you have to use dllexport to make them visible when building the dll.

dllimport is used when importing an exported symbol from a dll into another unit.

You often end up having a define to tell if you are building or using the dll, so you can switch between them in one header file.

nemequ commented 8 years ago

MSVC leaves all symbols hidden by default, and you have to use dllexport to make them visible when building the dll.

dllimport is used when importing an exported symbol from a dll into another unit.

I definitely misunderstood dllimport; I thought it was analogous to hidden visibility. e7fd3827 removes dllimport.

You often end up having a define to tell if you are building or using the dll, so you can switch between them in one header file.

Yep, that's what we do (based on whether SQUASH_COMPILATION is defined). 97caa6db changed how it is implemented slightly, but it's basically the same idea.

I'm not sure what the use case for dllimport is; based on the documentation you linked to it sounds like it's basically a Microsoft-specific version of the extern storage-class specifier from standard C. Right now SQUASH_API is defined as __attribute__((__default__)) when compiling Squash and extern otherwse; is there a reason to use __declspec(dllimport) instead of (or in addition to) extern?

jibsen commented 8 years ago

As I understand it, when you use dllimport, the compiler knows at compile time that it will be calling a function in a dll, and can potentially create better code for this (direct call). If you just use extern, the linker will find out that the function is located in a dll when it links with the import library.

nemequ commented 8 years ago

So

diff --git a/squash/squash.h b/squash/squash.h
index 304cfda..a882eff 100644
--- a/squash/squash.h
+++ b/squash/squash.h
@@ -116,9 +116,11 @@
 #  if defined(__GNUC__)
 #    define SQUASH_INTERNAL
 #    define SQUASH_EXTERNAL __attribute__((__dllexport__))
+#    define SQUASH_IMPORT   __attribute__((__dllimport__))
 #  else
 #    define SQUASH_INTERNAL
 #    define SQUASH_EXTERNAL __declspec(dllexport)
+#    define SQUASH_IMPORT   __declspec(dllimport)
 #  endif
 #else
 #  if defined(__GNUC__) && (__GNUC__ >= 4)
@@ -128,12 +130,13 @@
 #    define SQUASH_INTERNAL
 #    define SQUASH_EXTERNAL
 #  endif
+#  define SQUASH_IMPORT     extern
 #endif

 #if defined(SQUASH_COMPILATION)
 #  define SQUASH_API SQUASH_EXTERNAL
 #else
-#  define SQUASH_API extern
+#  define SQUASH_API SQUASH_IMPORT
 #endif

 #include <squash/version.h>

?

That way when we're not compiling Squash, on Windows we use dllimport, and everywhere else we use extern.

jibsen commented 8 years ago

Something along those lines yes. GCC supports the __declspec syntax as well for compatibility (at least since 3.3), so you probably do not need to special case it on Windows.

nemequ commented 8 years ago

Nice. Pushed (4caa1e5e), thanks.