Clipper2 icon indicating copy to clipboard operation
Clipper2 copied to clipboard

Fix Shared library export and don't install GTEST

Open damiandixon opened this issue 2 years ago • 4 comments

The shared library export fix is detailed in #244 for non Windows only.

This adds the CLIPPER2_DLL to all classes and methods that are in the headers that have a cpp implementation.

The CMakeLists.txt is updated to ensure that the exports are exported.

The libraries are built so that symbols are hidden unless exported.

Additional updates in CMakeLists.txt:

  • Added a default CMAKE_BUILD_TYPE of release and set the build type options to make it easier to select.
  • Turned off the install of GTEST as it makes no sense to install it.

This leaves the Windows build as a static library.

The default on Linux is a Shared library.

This is a work in progress.

damiandixon avatar Sep 30 '22 08:09 damiandixon

To get around the issue with the current build you will need to either disable the C4251 warning or prohibit the windows build from building a shared library.

To disable the C4251 warning you will need to sprinkle code like this throughout the code base inside class/struct declarations:

#pragma warning(push)
#pragma warning(disable : 4251)  // dllexport warning
...
#pragma warning(pop)

Here is a diff that fixes it:

git diff --staged
diff --git a/CPP/Clipper2Lib/clipper.engine.h b/CPP/Clipper2Lib/clipper.engine.h
index 5d28baa..5d3d647 100644
--- a/CPP/Clipper2Lib/clipper.engine.h
+++ b/CPP/Clipper2Lib/clipper.engine.h
@@ -20,6 +20,8 @@
 #include <functional>
 #include "clipper.core.h"

+#pragma warning(push)
+#pragma warning(disable : 4251)  // dllexport warning
 namespace Clipper2Lib {

        struct Scanline;
@@ -498,5 +500,6 @@ namespace Clipper2Lib {
        };

 }  // namespace
+#pragma warning(pop)

 #endif  // clipper_engine_h
diff --git a/CPP/Clipper2Lib/clipper.offset.h b/CPP/Clipper2Lib/clipper.offset.h
index d5399b4..1f91081 100644
--- a/CPP/Clipper2Lib/clipper.offset.h
+++ b/CPP/Clipper2Lib/clipper.offset.h
@@ -13,6 +13,8 @@

 #include "clipper.core.h"

+#pragma warning(push)
+#pragma warning(disable : 4251)  // dllexport warning
 namespace Clipper2Lib {

 enum class JoinType { Square, Round, Miter };
@@ -105,4 +107,6 @@ public:
 };

 }
+#pragma warning(pop)
+
 #endif /* CLIPPER_OFFSET_H_ */

lederernc avatar Sep 30 '22 14:09 lederernc

This adds the CLIPPER2_DLL to all classes and methods that are in the headers that have a cpp implementation.

I'm wondering if exporting classes is the best approach for DLLs? Would it not be better/safer to add additional (admittedly more complex) functions that encapsulate these classes; functions that provide the full feature set of these classes?

AngusJohnson avatar Sep 30 '22 23:09 AngusJohnson

This adds the CLIPPER2_DLL to all classes and methods that are in the headers that have a cpp implementation.

I'm wondering if exporting classes is the best approach for DLLs? Would it not be better/safer to add additional (admittedly more complex) functions that encapsulate these classes; functions that provide the full feature set of these classes?

Personally, I would prefer to keep the classes. All shared libraries in C++ bump into this in one form or another.

damiandixon avatar Oct 01 '22 13:10 damiandixon

Personally, I would prefer to keep the classes. All shared libraries in C++ bump into this in one form or another.

I appreciate that exporting C++ classes may be very useful in Linux, but ISTM that it's discouraged in Windows. And is this approach really better than providing a small set of exported functions that will do everything that's done by these classes?

AngusJohnson avatar Oct 02 '22 10:10 AngusJohnson