Shared library #243

sergey-239 · 2022-09-29T10:26:23Z

Angus,

please review the last change of the last commit in this PR: I hope I got the original intent properly

Thank you,
Sergey

BugFix: evaluation order of `*c = case_table[*c++];` expression is undefined

AngusJohnson · 2022-09-29T11:02:58Z

Thanks Sergey. I'm not doubting your changes but since I'm not competent to critique them, I'm hoping someone who's already contributed to Clipper2's makefile (@lederernc, @reunanen & @damiandixon) will have a quick look before I commit. Cheers.

sergey-239 · 2022-09-29T11:27:09Z

Thanks Sergey. I'm not doubting your changes but since I'm not competent to critique them, I'm hoping someone who's already contributed to Clipper2's makefile (@lederernc, @reunanen & @damiandixon) to have a quick look before I commit. Cheers.

this is fine with me.

Regards,
Sergey

sergey-239 · 2022-09-29T11:48:55Z

The Fedora package build result could be seen here:
https://copr.fedorainfracloud.org/coprs/snmende/libClipper2/build/4878266/

jdumas · 2022-09-29T14:50:53Z

CPP/CMakeLists.txt

@@ -25,21 +28,19 @@ set(CLIPPER2_SRC
 )

 # 2d version of Clipper2
-add_library(Clipper2 STATIC ${CLIPPER2_INC} ${CLIPPER2_SRC})
+add_library(Clipper2 ${CLIPPER2_SRC})


Removing the STATIC is a good idea, but for some IDE (like Visual Studio), it's nice to add the include files to the library sources (the PUBLIC_HEADER property is only relevant for installing targets on macOS).

Does VS use cmake files nowadays?
No, not a problem for me to bring the header lists back to add_libray's, I'm just curious

Yes you can, but I generally let CMake generate the Visual Studio project for me. When header files are properly listed in the target sources they show us along with other .cpp. Otherwise they get mixed up with system headers and such.

Clipper2 will not compile as a DLL with all the appropriate warning flags as errors in Visual Studio. This is because the library needs to export classes that contain std::vector which is "not a good idea" on Windows.

@lederernc, then we could make this conditionally disabled for WIN32 builds.

jdumas · 2022-09-29T14:55:19Z

CPP/CMakeLists.txt

-  DESTINATION include
-  FILES_MATCHING PATTERN "*.h"
+install(TARGETS Clipper2 Clipper2Z
+        PUBLIC_HEADER DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/clipper2


This change will solve #232 when an installed version of clipper2 is used, but it will not do anything when clipper2 is used as a subproject (e.g. via FetchContent + add_subdirectory()).

reunanen · 2022-09-29T15:32:51Z

CPP/Utils/ClipFileSave.cpp

@@ -64,7 +64,7 @@ class BMH_Search
  {
    while (current < end)
    {
-      uint8_t i = shift[*current];  //compare last byte first
+      uint8_t i = shift[(unsigned)*current];  //compare last byte first


I would prefer more C++-y typecasting, e.g. static_cast<unsigned>(*current).

I would also prefer more modern typecasting

AngusJohnson · 2022-09-30T00:20:40Z

Thanks everyone for all the helpful feedback.
Sergey, would you be happy to revise this PR to accommodate this feedback or create a new PR? Cheers.

sergey-239 · 2022-09-30T00:31:17Z

Angus, yes, I could do this. We just need to define the new directory structure as @jdumas pointed scenarios where it is important. How about this:

CPP/Clipper2Lib
├── include
│   └── clipper2
└── src

These three new are subdirs of Clipper2Lib

AngusJohnson · 2022-09-30T01:19:02Z

CPP/Clipper2Lib
            ├── include
            │       └── clipper2
            └── src

I'm presuming that we're just talking about the files that are currently in Clipper2Lib.
I'm also presuming that the .h files go in include and the .cpp files into src.
But I don't understand why you need a subdirectory under include?

sergey-239 · 2022-09-30T01:33:38Z

Angus,
it is because you would put the <clipper2/clipper.h>, for example, into #include directive. To let the compiler treat this properly you should use the path to a directory that contains the clipper2 directory as argument of the -I compiler option: e.g.:
gcc mytest,cpp -I ./Clipper2Lib/include
So, when the headers installed into the system include directory the same source file compiles with just default compiler includes search path.
Probably, we could leave the source files in the Clipper2Lib, create one subdirectory clipper2 under it and move headers into it. But then, the Clipper2Lib would be searchable for include files during build of library itself, tools and tests.
I see the latter solution as a possible source of compilation problems, but it might be that my approach is an overkill.
Any way, it is completely up to you to decide.

Thank you
Sergey

AngusJohnson · 2022-09-30T02:01:57Z

it is because you would put the <clipper2/clipper.h>, for example, into #include directive.

OK, I think I get what you're suggesting now.

CPP/Clipper2Lib
            ├── include
            │       │ 
            │       └── clipper2
            │               clipper.h
            │               clipper.core.h
            │               clipper.engine.h
            │               clipper.offset.h
            │               clipper.minkowski.h
            └── src
                  clipper.engine.cpp
                  clipper.offset.cpp

I don't really follow why/how this structure helps, but I'm very happy to go with it and trust you and others who have a much better understanding of C++.

sergey-239 · 2022-09-30T02:10:01Z

Yes, exactly.
I'll give a time to others to comment.
Thanks you

AngusJohnson · 2022-09-30T02:49:45Z

Yes, exactly.
I'll give a time to others to comment.

Great. Thanks.
And just for my education ... why is the clipper2 subdirectory of include helpful?
Is it simply that a raw #include "clipper.h" (ie without a preceding directory) would imply a core library?

sergey-239 · 2022-09-30T03:31:16Z

It's useful because you might have a few projects under development, part of which depends on Clipper1 and the other part on Clipper2 library.
For example,

rpm -ql polyclipping-devel-6.4.2-14.fc36.x86_64
/usr/include/polyclipping
/usr/include/polyclipping/agg_conv_clipper.h
/usr/include/polyclipping/clipper.hpp
/usr/lib64/libpolyclipping.so

your original clipper library packaged for Fedora with this directory layout as there were no established one.

Now you may define that Clipper2 include files are in clipper2 subfolder of the standard include path, so when your new library would spread over different unix flavours, it will be common to use #include <clipper2/clipper.h> or whatever include file you would provide in the course of lib development. Of course, you could encode the lib "generation" number into file names but this is usually less convenient.

By the way I have one project I would like to package for Fedora, that uses some strange version of clipper that I can't identify:

ClipperBase.cpp
ClipperBase.h
Clipper.cpp
Clipper.h
Enum.h
Exception.h
Int128.cpp
Int128.h
IntersectNode.cpp
IntersectNode.h
IntPoint.cpp
IntPoint.h
OffsetBuilder.cpp
OffsetBuilder.h
OutPt.cpp
OutPt.h
OutRec.cpp
OutRec.h
Polygon.cpp
Polygon.h
Polygons.cpp
Polygons.h
PolyNode.cpp
PolyNode.h
PolyTree.cpp
PolyTree.h
TEdge.cpp
TEdge.h
Util.h

All these files copyrighted by you and have version 5.1.6
Curious?

AngusJohnson · 2022-09-30T03:46:49Z

It's useful because you might have a few projects under development, part of which depends on Clipper1 and the other part on Clipper2 library.

OK. That does make sense.

By the way I have one project I would like to package for Fedora, that uses some strange version of clipper that I can't identify:

It looks like someone (not me) split my very large clipper.hpp and clipper.cpp files into lots of smaller ones.
I see advantages and disadvantages to this but lots of little files seems more common in C++.

damiandixon · 2022-09-30T07:00:46Z

CPP/CMakeLists.txt

@@ -10,6 +10,7 @@ set_property(GLOBAL PROPERTY USE_FOLDERS ON)
 option(CLIPPER2_UTILS "Build utilities" ON)
 option(CLIPPER2_EXAMPLES "Build examples" ON)
 option(CLIPPER2_TESTS "Build tests" ON)
+option(BUILD_SHARED_LIBS "Build shared libs" OFF)


I would suggest shared library is on by default.

The original cmake was to build a shared library but was changed in #169 to static.

The DLL export on Windows requires the classes and methods to be exported using:

#ifndef CLIPPER2_DLL # if defined(_WIN32) # ifdef CLIPPER2_DLL_EXPORT # define CLIPPER2_DLL __declspec(dllexport) # else # define CLIPPER2_DLL __declspec(dllimport) # endif # else # if __GNUC__ >= 4 # define CLIPPER2_DLL __attribute__((visibility("default"))) # else # define CLIPPER2_DLL # endif # endif #endif

With classes defined:

class CLIPPER2_DLL Clipper2Class ....

When building the Define CLIPPER2_DLL_EXPORT is declared.

Probably should be a separate issue.

Raised #244 for exporting symbols in a shared library.

I've done a pull request #245 dealing with shared libraries

AngusJohnson · 2022-09-30T09:46:49Z

OK, I really do appreciate all the advice that flowing in re C++ packaging. Unfortunately however, I'm not experienced enough in C++ to sensibly navigate all the proposed changes. Sergey or Damian, would either of you be happy to coordinate this current flurry of activity re C++ packaging? If so, that'd be a very big help.

damiandixon · 2022-09-30T10:11:52Z

OK, I really do appreciate all the advice that flowing in re C++ packaging. Unfortunately however, I'm not experienced enough in C++ to sensibly navigate all the proposed changes. Sergey or Damian, would either of you be happy to coordinate this current flurry of activity re C++ packaging? If so, that'd be a very big help.

I don't have an issue with the changes here as they move things in the right direction. TBH I'm no expert on cmake either, as I'm stumped as to how to deal with #243 (comment) as I normally use submodules with git.

There are issues around shared libraries and Windows builds that I'm looking at on pull request #245, though I suspect #243 (comment) may mean a shared library on Windows is difficult or a non-starter. I would suggest using the simplest approach to shared libraries for Linux and stick to static on Windows for this update.

sergey-239 · 2022-09-30T12:51:10Z

OK, I really do appreciate all the advice that flowing in re C++ packaging. Unfortunately however, I'm not experienced enough in C++ to sensibly navigate all the proposed changes. Sergey or Damian, would either of you be happy to coordinate this current flurry of activity re C++ packaging? If so, that'd be a very big help.

I don't have an issue with the changes here as they move things in the right direction. TBH I'm no expert on cmake either, as I'm stumped as to how to deal with #243 (comment) as I normally use submodules with git.

There are issues around shared libraries and Windows builds that I'm looking at on pull request #245, though I suspect #243 (comment) may mean a shared library on Windows is difficult or a non-starter. I would suggest using the simplest approach to shared libraries for Linux and stick to static on Windows for this update.

Ok, then I can redo the patch taking to account discussion above. I don't see an issue with default static build configuration for all platforms: if one need the shared lib then it will be just one option to cmake to get shared versions. Just like me: it's Fedora preference to package separately as much as possible as shared libraries to minimise licensing issues.

AngusJohnson · 2022-09-30T13:16:45Z

Thanks Sergey.

sergey-239 · 2022-10-01T04:23:27Z

Angus,

did you merge this pr intentionally?
I meant that I will remake all changes from the point where I branched from main trunk.
Please, clarify

Thank you
Sergey.

AngusJohnson · 2022-10-01T04:29:52Z

did you merge this pr intentionally?

Sorry, it was late at night and I "jumped the gun" 😱.
Nevertheless, I think it's OK as it is (at least until you're ready to complete).
But if you'd prefer I can revert.

sergey-239 · 2022-10-01T04:36:08Z

I am not sure: I actually don't know if github allows to do reset --hard on last commit. If it would do an actual revert, i.e. it will produce a new commit, then no, this would only pollute the commit history even more.
So if it can't do reset --hard HEAD~1 then I manage myself.

AngusJohnson · 2022-10-01T04:56:14Z

Sergey, as you can see, I've just given you temporary collaborator access so feel free to manage this as you think best 😁.

sergey-239 · 2022-10-01T05:04:47Z

Found this:

Can you make a positive commit to fix the problem and what is the fix class?

Rewriting public history is a bad idea. It requires everyone else to do special things and you must publicly announce your failure. Ideally you will create either a commit to just fix the problem, or a new git revert commit to create a new commit which undoes what the commit target of the revert did.

So, I will manage myself. No action required.

* Allow building as a shared lib * Disable annoying `unused func/var` warnings in utils & tests * Fix headers installation destination & cleanup of CMakeLists.txt * BugFix: unsigned char promotes to int by C and C++ standards BugFix: evaluation order of `*c = case_table[*c++];` expression is undefined

* Fixes for #243 as per discussion * Move files as per #243 discussion * Fix include paths in Examples * * Replace relative include paths in examples * Disable unused-(function|result) warning for gcc targets * Fix some indentation in CMakeLists.txt * - Add pkgconfig - Fix test in CMakeLists.txt for actual gcc use

sergey-239 added 4 commits September 29, 2022 13:00

Allow building as a shared lib

583f674

Disable annoying unused func/var warnings in utils & tests

8452c53

Fix headers installation destination & cleanup of CMakeLists.txt

87b1e02

BugFix: unsigned char promotes to int by C and C++ standards

4f8be4b

BugFix: evaluation order of `*c = case_table[*c++];` expression is undefined

sergey-239 mentioned this pull request Sep 29, 2022

Use Clipper2Lib/ subfolder for public headers? #232

Closed

jdumas reviewed Sep 29, 2022

View reviewed changes

reunanen reviewed Sep 29, 2022

View reviewed changes

damiandixon reviewed Sep 30, 2022

View reviewed changes

AngusJohnson merged commit c9de2eb into AngusJohnson:main Sep 30, 2022

sergey-239 deleted the shared_library branch October 1, 2022 03:58

sergey-239 restored the shared_library branch October 1, 2022 04:08

sergey-239 added a commit to sergey-239/Clipper2 that referenced this pull request Oct 1, 2022

Fixes for AngusJohnson#243 as per discussion

b520a63

sergey-239 added a commit to sergey-239/Clipper2 that referenced this pull request Oct 1, 2022

Move files as per AngusJohnson#243 discussion

75bddab

sergey-239 deleted the shared_library branch October 2, 2022 01:43

aismann mentioned this pull request Oct 4, 2022

Clipper2 1.05 (only files with changes on source lines) axmolengine/axmol#892

Merged

2 tasks

Shared library #243

Shared library #243

Conversation

sergey-239 commented Sep 29, 2022

AngusJohnson commented Sep 29, 2022 • edited Loading

sergey-239 commented Sep 29, 2022

sergey-239 commented Sep 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdumas Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lederernc Sep 29, 2022 • 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

AngusJohnson commented Sep 30, 2022

sergey-239 commented Sep 30, 2022 • edited Loading

AngusJohnson commented Sep 30, 2022

sergey-239 commented Sep 30, 2022 • edited Loading

AngusJohnson commented Sep 30, 2022 • edited Loading

sergey-239 commented Sep 30, 2022

AngusJohnson commented Sep 30, 2022

sergey-239 commented Sep 30, 2022 • edited Loading

AngusJohnson commented Sep 30, 2022

damiandixon Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damiandixon Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

AngusJohnson commented Sep 30, 2022

damiandixon commented Sep 30, 2022 • edited Loading

sergey-239 commented Sep 30, 2022

AngusJohnson commented Sep 30, 2022

sergey-239 commented Oct 1, 2022 • edited Loading

AngusJohnson commented Oct 1, 2022 • edited Loading

sergey-239 commented Oct 1, 2022 • edited Loading

AngusJohnson commented Oct 1, 2022

sergey-239 commented Oct 1, 2022

Can you make a positive commit to fix the problem and what is the fix class?

AngusJohnson commented Sep 29, 2022 •

edited

Loading

jdumas Sep 29, 2022 •

edited

Loading

lederernc Sep 29, 2022 •

edited

Loading

sergey-239 commented Sep 30, 2022 •

edited

Loading

sergey-239 commented Sep 30, 2022 •

edited

Loading

AngusJohnson commented Sep 30, 2022 •

edited

Loading

sergey-239 commented Sep 30, 2022 •

edited

Loading

damiandixon Sep 30, 2022 •

edited

Loading

damiandixon Sep 30, 2022 •

edited

Loading

damiandixon commented Sep 30, 2022 •

edited

Loading

sergey-239 commented Oct 1, 2022 •

edited

Loading

AngusJohnson commented Oct 1, 2022 •

edited

Loading

sergey-239 commented Oct 1, 2022 •

edited

Loading