-
Notifications
You must be signed in to change notification settings - Fork 417
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
C++ visibility support #231
Conversation
################################################# | ||
# Macro to check for visibility capability in compiler | ||
# Original idea from: https://gitorious.org/ferric-cmake-stuff/ | ||
macro (check_gcc_visibility) |
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.
This also works for clang (and maybe other compilers), so maybe rename to check_compiler_visibility?
*/ | ||
|
||
#if defined _WIN32 || defined __CYGWIN__ | ||
#ifdef BUILDING_DLL |
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.
In Gazebo, the usage of the same BUILDING_DLL
macro for different libraries created several problems (see https://bitbucket.org/osrf/gazebo/issues/2262/symbol-visibility-issues-in-windows). I would recommend to use a fcl-specific macro and set the DEFINE_SYMBOL
property of the library appropriately.
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.
The header in this form is breaking the static builds in MSVC, that is the default behaviour when building the library in MSVC.
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.
Oh sorry forgot about this issue, thanks for the reminder Silvio. Could please give a look at this patch j-rivero@e509a3d and let me know if it is correct? Unfortunately I don't have a Windows/MSVC15 setup at hand to test.
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.
Commented in j-rivero@e509a3d .
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.
Sorry, I actually commented in e509a3d .
@@ -52,9 +52,9 @@ namespace detail | |||
//static const size_t EPA_MAX_ITERATIONS = 255; | |||
// TODO(JS): remove? | |||
|
|||
/// @brief class for EPA algorithm | |||
/// @brief class FCL_VISIBLE for EPA algorithm |
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.
There are a few spurious insertion of the FCL_VISIBLE
macro in the documentation, they can be easily found by searching for FCL_VISIBLE for
.
Ouk, sorry I've pushed the mess of my working copy to the repo (reverts on reverts). I'll open a new pull request with a clean history and close this one. |
Excuse me for the change of PR. Let's follow in #233 |
This PR implements the control of binary symbols visibility in FCL.
It adds a new
FCL_VISIBLE
preprocessor define to any class, struct or function that needs to be exposed publicly to third party software. Since the number of files in this library is not small, I've patched all what I found needed to compile the whole test suite. It would be good to use another third party software using FCL extensively to test that I did not forget about any of them.The PR also adds the
FCL_HIDE_ALL_SYMBOLS
option (OFF by the default). This option covers a use case we found in Drake where a software uses FCL (linked statically) but does not expose it in the public API (headers). In this scenario the best option to avoid any possible conflict with other versions of fcl is to hide all the symbols from the public ABI.Numbers before and after:
Summary: 13% symbols less and 6% size less .
The PR will alter the ABI so a new bump in the FCL version prior to release it would be ideal.