C++ visibility support (take II) #233

j-rivero · 2017-10-11T14:49:36Z

Continues discussion and work done in #231. Reproducing the summary:

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:

Version	Number of symbols	libfcl size
current	18942	11922 Kb
with visibility from this PR	16530	11257Kb
no symbols	157	9012Kb

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.

j-rivero · 2017-10-14T17:36:59Z

I think that all the compilers/options on Linux and Windows are now happy. Mac is failing due to a problem finding ccd (I think it is unrelated to this PR).

j-rivero · 2017-11-09T12:27:33Z

@jslee02 I've been trying to compile this branch with Dart (master and tag 6.3.1) but there are compilation failures due to a change in headers location. Do you know if there is any branch ready in Dart ready to work with fcl current master?

jslee02 · 2017-11-09T14:37:16Z

@j-rivero dartsim/dart#873 should work with this PR. I tested it against FCL 0.5 as well.

jslee02

I'm not an expert on exporting symbols, but this looks a great change according to the description!

I've made some comments:

I guess you replaced "class " with "class FCL_EXPOSE ". It seems this made unintended changes in descriptions of some classes.
About the name and installation location of fcl_expose.h.

Besides on that, it looks good to me.

jslee02 · 2017-11-10T14:41:56Z

include/fcl/geometry/bvh/BV_node.h

@@ -47,9 +47,9 @@
 namespace fcl
 {

-/// @brief A class describing a bounding volume node. It includes the tree structure providing in BVNodeBase and also the geometry data provided in BV template parameter.
+/// @brief A class FCL_EXPORT describing a bounding volume node. It includes the tree structure providing in BVNodeBase and also the geometry data provided in BV template parameter.


Maybe an oversight?

jslee02 · 2017-11-10T14:42:17Z

include/fcl/geometry/bvh/detail/BV_splitter.h

@@ -60,9 +60,9 @@ enum SplitMethodType
  SPLIT_METHOD_BV_CENTER
 };

-/// @brief A class describing the split rule that splits each BV node
+/// @brief A class FCL_EXPORT describing the split rule that splits each BV node


Maybe an oversight?

jslee02 · 2017-11-10T14:43:14Z

include/fcl/geometry/shape/utility.h

@@ -38,7 +38,7 @@
 #ifndef FCL_GEOMETRY_SHAPE_UTILITY_H
 #define FCL_GEOMETRY_SHAPE_UTILITY_H

-// This header shouldn't be included by any bounding volumen classes (e.g.,
+// This header shouldn't be included by any bounding volumen class FCL_EXPORTes (e.g.,


Maybe an oversight?

jslee02 · 2017-11-10T14:43:31Z

include/fcl/math/bv/AABB.h

@@ -43,10 +43,10 @@
 namespace fcl
 {

-/// @brief A class describing the AABB collision structure, which is a box in 3D
+/// @brief A class FCL_EXPORT describing the AABB collision structure, which is a box in 3D


Maybe an oversight?

jslee02 · 2017-11-10T14:43:44Z

include/fcl/math/bv/OBB.h

@@ -46,9 +46,9 @@
 namespace fcl
 {

-/// @brief Oriented bounding box class
+/// @brief Oriented bounding box class FCL_EXPORT


Maybe an oversight?

jslee02 · 2017-11-10T15:02:46Z

CMakeLists.txt

+# Need to include directory to find export file
+include_directories(${PROJECT_BINARY_DIR}) 
+
+install(FILES ${PROJECT_BINARY_DIR}/${PROJECT_NAME}_export.h


Could we change the export header to simply export.h? I don't see any ambiguity or confliction in including this file if we install this file under ${CMAKE_INSTALL_INCLUDEDIR}/fcl. In this way, we could include this file as:

#include "fcl/export.h" // in FCL code base #include <fcl/export.h> // in FCL dependent projects.

jslee02 · 2017-11-10T15:04:31Z

CMakeLists.txt

@@ -42,6 +45,7 @@ include(FCLMacros)
 include(CompilerSettings)
 include(FCLVersion)
 include(GNUInstallDirs)
+include(GenerateExportHeader) 


I just learned this very useful CMake utility! 😮

jslee02 · 2017-11-10T15:10:46Z

include/fcl/narrowphase/detail/gjk_solver_indep.h

 {
  using S = S_;

  /// @brief intersection checking between two shapes
  /// @deprecated use shapeIntersect(const Shape1&, const Transform3<S>&, const Shape2&, const Transform3<S>&, std::vector<ContactPoint<S>>*) const
  template<typename Shape1, typename Shape2>
-  FCL_DEPRECATED


Is this an oversight or removed intentionally? fcl_export.h still has the definition of FCL_DEPRECATED. This deprecation was introduced in #52 for backward compatibility. If we want to remove this deprecation, I would like to remove this function entirely not to cause confusion of which shapeIntersect should be used.

jslee02 · 2017-11-10T15:12:38Z

include/fcl/narrowphase/detail/gjk_solver_libccd.h

 {
  using S = S_;

  /// @brief intersection checking between two shapes
  /// @deprecated use shapeIntersect(const Shape1&, const Transform3<S>&, const Shape2&, const Transform3<S>&, std::vector<ContactPoint<S>>*) const
  template<typename Shape1, typename Shape2>
-  FCL_DEPRECATED


ditto:

Is this an oversight or removed intentionally? fcl_export.h still has the definition of FCL_DEPRECATED. This deprecation was introduced in #52 for backward compatibility. If we want to remove this deprecation, I would like to remove this function entirely not to cause confusion of which shapeIntersect should be used.

jslee02 · 2017-11-10T15:13:07Z

include/fcl/narrowphase/detail/traversal/distance/mesh_distance_traversal_node.h

@@ -212,7 +216,7 @@ bool initialize(
    DistanceResult<S>& result);

 template <typename BV>
-FCL_DEPRECATED
+FCL_EXPORT


I guess this should be FCL_DEPRECATED_EXPORT.

j-rivero · 2017-11-14T22:38:23Z

Thanks JS for the review.

I guess you replaced "class " with "class FCL_EXPOSE ". It seems this made unintended changes in descriptions of some classes.

You were right. I think I've reverted all the changes you detected. Thanks for carefully looking into them.

About the name and installation location of fcl_expose.h.

I agree, we can use fcl/export.h. I've modified the code in f98dd43 to do it.

jslee02

Looks good to me!

j-rivero · 2017-11-16T19:51:10Z

While testing DART compilation against this branch I found a little problem with export.h location and fixed it in 831a13e. In my opinion, we are ready to merge.

jslee02 · 2017-11-16T21:56:57Z

While testing DART compilation against this branch I found a little problem with export.h location and fixed it in 831a13e.

Good catch! Let's merge this unless we have more feedback by tomorrow.

j-rivero added 4 commits October 4, 2017 14:04

Implement C++ visibility

43d6a4e

Added an option to hide all symbols

17a6c33

Use compiler instead of gcc for variable names

3fb1525

Revert wrong changes introduced in headers documentation

59e3f2d

j-rivero mentioned this pull request Oct 11, 2017

C++ visibility support #231

Closed

j-rivero and others added 8 commits October 11, 2017 19:52

Add missing FCL_VISIBLE macros

c8c6d50

More FCL_VISIBLE additions

9fa581d

Merge branch 'master' into visibility2

d090a99

Missing symbols

268af4c

Last two missing FCL_VISIBLE symbols

3751298

Merge branch 'visibility2' of github.com:j-rivero/fcl into visibility2

22a8fb6

Implement the use of GenerateExportHeader to control visibility

4c9e019

Added missing macros to make clang happy

23afecf

stonier mentioned this pull request Nov 6, 2017

Resolving system dependency conflicts (meta) RobotLocomotion/drake#7412

Closed

17 tasks

j-rivero mentioned this pull request Nov 7, 2017

Cherry pick visibilty support from upstream RobotLocomotion/fcl#1

Closed

Fix the definition after using GenerateExportHeader

956d3a9

Merge branch 'master' into visibility2

3a24d6f

jslee02 reviewed Nov 10, 2017

View reviewed changes

jslee02 added this to the FCL 0.6.0 milestone Nov 10, 2017

jslee02 and others added 4 commits November 10, 2017 16:00

Merge branch 'master' into visibility2

b511676

Change export header from fcl_export.h to fcl/export.h

f98dd43

Revert unintended modifications

a232b08

Merge branch 'visibility2' of github.com:j-rivero/fcl into visibility2

fd81e43

Merge branch 'master' into visibility2

c367a8b

jslee02 approved these changes Nov 16, 2017

View reviewed changes

Fix install directory of the export.h file

831a13e

Merge branch 'visibility2' of github.com:j-rivero/fcl into visibility2

2b2478d

jslee02 merged commit 4304833 into flexible-collision-library:master Nov 17, 2017

jslee02 mentioned this pull request Mar 22, 2018

C++ visibility support dartsim/dart#1040

Closed

4 tasks

mamoll mentioned this pull request Sep 20, 2019

Mark all dll exports with cmakes GenerateExportHeader ompl/ompl#507

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C++ visibility support (take II) #233

C++ visibility support (take II) #233

j-rivero commented Oct 11, 2017

j-rivero commented Oct 14, 2017

j-rivero commented Nov 9, 2017

jslee02 commented Nov 9, 2017

jslee02 left a comment •

edited

Loading

jslee02 Nov 10, 2017

jslee02 Nov 10, 2017

jslee02 Nov 10, 2017

jslee02 Nov 10, 2017

jslee02 Nov 10, 2017

jslee02 Nov 10, 2017

jslee02 Nov 10, 2017 •

edited

Loading

jslee02 Nov 10, 2017

jslee02 Nov 10, 2017

jslee02 Nov 10, 2017

j-rivero commented Nov 14, 2017

jslee02 left a comment

j-rivero commented Nov 16, 2017

jslee02 commented Nov 16, 2017

C++ visibility support (take II) #233

C++ visibility support (take II) #233

Conversation

j-rivero commented Oct 11, 2017

j-rivero commented Oct 14, 2017

j-rivero commented Nov 9, 2017

jslee02 commented Nov 9, 2017

jslee02 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jslee02 Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-rivero commented Nov 14, 2017

jslee02 left a comment

Choose a reason for hiding this comment

j-rivero commented Nov 16, 2017

jslee02 commented Nov 16, 2017

jslee02 left a comment •

edited

Loading

jslee02 Nov 10, 2017 •

edited

Loading