Skip to content
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.

Sign up for GitHub

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

Shared library #243

Merged
merged 4 commits into from
Sep 30, 2022
Merged

Shared library #243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
583f674
Allow building as a shared lib
sergey-239 Sep 29, 2022
8452c53
Disable annoying `unused func/var` warnings in utils & tests
sergey-239 Sep 29, 2022
87b1e02
Fix headers installation destination & cleanup of CMakeLists.txt
sergey-239 Sep 29, 2022
4f8be4b
BugFix: unsigned char promotes to int by C and C++ standards
sergey-239 Sep 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 24 additions & 17 deletions CPP/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
cmake_minimum_required(VERSION 3.10)
project(Clipper2 LANGUAGES C CXX)
project(Clipper2 VERSION 1.0.4 LANGUAGES C CXX)

set(CMAKE_POSITION_INDEPENDENT_CODE ON)
set(CMAKE_CXX_STANDARD 17)
Expand All @@ -10,6 +10,9 @@ 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)
Copy link
Contributor

@damiandixon damiandixon Sep 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raised #244 for exporting symbols in a shared library.

Copy link
Contributor

@damiandixon damiandixon Sep 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


include(GNUInstallDirs)

set(CLIPPER2_INC
Clipper2Lib/clipper.core.h
Expand All @@ -25,21 +28,19 @@ set(CLIPPER2_SRC
)

# 2d version of Clipper2
add_library(Clipper2 STATIC ${CLIPPER2_INC} ${CLIPPER2_SRC})
add_library(Clipper2 ${CLIPPER2_SRC})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@jdumas jdumas Sep 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now.

Copy link
Contributor

@lederernc lederernc Sep 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


target_include_directories(Clipper2
PUBLIC Clipper2Lib
SYSTEM INTERFACE Clipper2Lib
)

# Clipper2 but with USINGZ defined
add_library(Clipper2Z STATIC ${CLIPPER2_INC} ${CLIPPER2_SRC})
add_library(Clipper2Z ${CLIPPER2_SRC})

target_compile_definitions(Clipper2Z PUBLIC USINGZ)

target_include_directories(Clipper2Z
PUBLIC Clipper2Lib
SYSTEM INTERFACE Clipper2Lib
)

if (WIN32)
Expand All @@ -53,7 +54,11 @@ else()
target_link_libraries(Clipper2Z PUBLIC -lm)
endif()

set_target_properties(Clipper2 Clipper2Z PROPERTIES FOLDER Libraries)
set_target_properties(Clipper2 Clipper2Z PROPERTIES FOLDER Libraries
VERSION ${PROJECT_VERSION}
SOVERSION ${PROJECT_VERSION_MAJOR}
PUBLIC_HEADER "${CLIPPER2_INC}"
)

if(CLIPPER2_UTILS OR CLIPPER2_TESTS OR CLIPPER2_EXAMPLES)
set(CLIPPER2_UTILS_INC
Expand All @@ -67,24 +72,25 @@ if(CLIPPER2_UTILS OR CLIPPER2_TESTS OR CLIPPER2_EXAMPLES)
Utils/ClipFileSave.cpp
)

add_library(Clipper2utils STATIC ${CLIPPER2_UTILS_INC} ${CLIPPER2_UTILS_SRC})
add_library(Clipper2utils STATIC ${CLIPPER2_UTILS_SRC})

target_link_libraries(Clipper2utils PUBLIC Clipper2)
target_include_directories(Clipper2utils
PUBLIC Utils
SYSTEM INTERFACE Utils
)

add_library(Clipper2Zutils STATIC ${CLIPPER2_UTILS_INC} ${CLIPPER2_UTILS_SRC})
add_library(Clipper2Zutils STATIC ${CLIPPER2_UTILS_SRC})

target_link_libraries(Clipper2Zutils PUBLIC Clipper2Z)
target_include_directories(Clipper2Zutils
PUBLIC Utils
SYSTEM INTERFACE Utils
)

set_target_properties(Clipper2utils Clipper2Zutils PROPERTIES FOLDER Libraries)

if (NOT WIN32)
target_compile_options(Clipper2utils PRIVATE -Wno-unused-variable -Wno-unused-function)
target_compile_options(Clipper2Zutils PRIVATE -Wno-unused-variable -Wno-unused-function)
endif()
endif()

if(CLIPPER2_EXAMPLES)
Expand Down Expand Up @@ -140,6 +146,11 @@ if(CLIPPER2_TESTS)
add_executable(ClipperTestsZ ${ClipperTests_SRC})
target_link_libraries(ClipperTestsZ gtest gtest_main Clipper2Z Clipper2Zutils)

if (NOT WIN32)
target_compile_options(ClipperTests PRIVATE -Wno-unused-variable -Wno-unused-function)
target_compile_options(ClipperTestsZ PRIVATE -Wno-unused-variable -Wno-unused-function)
endif()

set_target_properties(ClipperTests ClipperTestsZ PROPERTIES FOLDER Tests)

gtest_discover_tests(ClipperTests
Expand All @@ -161,10 +172,6 @@ if(CLIPPER2_TESTS)
file(COPY ../Tests/Polygons.txt DESTINATION ${CMAKE_BINARY_DIR} FILE_PERMISSIONS OWNER_READ GROUP_READ WORLD_READ )
endif()

install(TARGETS Clipper2 Clipper2Z)

install(
DIRECTORY Clipper2Lib/
DESTINATION include
FILES_MATCHING PATTERN "*.h"
install(TARGETS Clipper2 Clipper2Z
PUBLIC_HEADER DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/clipper2
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()).

)
10 changes: 5 additions & 5 deletions CPP/Utils/ClipFileSave.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also prefer more modern typecasting

if (!i) //last byte matches if i == 0
{
char* j = current - needle_len_less1;
Expand All @@ -88,12 +88,12 @@ class BMH_Search
{
while (current < end)
{
uint8_t i = shift[case_table[*current]];
uint8_t i = shift[case_table[(unsigned)*current]];
if (!i)
{
char* j = current - needle_len_less1;
while (i < needle_len_less1 &&
needle_ic_[i] == case_table[*(j + i)]) ++i;
needle_ic_[i] == case_table[(unsigned)*(j + (unsigned)i)]) ++i;
if (i == needle_len_less1)
{
++current;
Expand Down Expand Up @@ -154,8 +154,8 @@ class BMH_Search
needle_ic_ = new uint8_t[needle_len_ +1];
std::memcpy(needle_ic_, needle_, needle_len_);
uint8_t* c = needle_ic_;
for (uint8_t i = 0; i < needle_len_; ++i)
*c = case_table[*c++];
for (uint8_t i = 0; i < needle_len_; ++i, c++)
*c = case_table[*c];

std::fill(std::begin(shift), std::begin(shift) + 256, needle_len_);
for (uint8_t j = 0; j < needle_len_less1; ++j)
Expand Down