Add CMake build system and cross-platform compatibility#5
Add CMake build system and cross-platform compatibility#5keisuke-ishihara wants to merge 4 commits intomorphometry:masterfrom
Conversation
Replace POSIX mkdir() with std::filesystem::create_directories() and fix platform-specific issues (MSVC alternative tokens, arm64 debug trap, missing <sstream> include) to enable building on Windows, macOS arm64, and Linux. Add GitHub Actions CI workflow and vcpkg manifest. Tested: builds and passes all tests on macOS arm64 (Apple Silicon). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Build and tests confirmed successful on Windows with make using MINGW64. This flag is a no-op on macOS/Linux where M_PI is already defined. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… MSVC build MSVC (C2765) disallows default arguments on explicit template instantiations. The defaults are already declared in the header (calculate_wxxx.h), so removing them from the .cpp instantiations is standards-compliant and cross-platform. Tested with Windows+CMake (MSVC), MinGW, and macOS builds. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
hi, thanks for creating this package. We are using it for our research and it's been very useful! |
skapfer
left a comment
skapfer
left a comment
There was a problem hiding this comment.
Thank you for the pull request! It looks pretty good, some small changes might be in order.
Makefile
Outdated
| CXX = g++ | ||
|
|
||
| CXXFLAGS = -Wall -O2 -DNDEBUG #-ansi | ||
| CXXFLAGS = -std=c++17 -Wall -O2 -DNDEBUG -D_USE_MATH_DEFINES #-ansi |
There was a problem hiding this comment.
Can we use a more portable fix and include this after the <cmath> include?
#ifndef M_PI
#define M_PI 3.14159265358979323846
#endif
There was a problem hiding this comment.
Ok I removed -D_USE_MATH_DEFINES from both Makefile and CMakeLists.txt, and added #ifndef M_PI after <math.h> in each file that uses it.
|
|
||
| static | ||
| void throw_error (const std::string &message, ...) { | ||
| void throw_error (const std::string message, ...) { |
There was a problem hiding this comment.
Why did this need to change?
There was a problem hiding this comment.
The change is needed for MSVC compatibility. Per the C++ standard, passing a reference as the last named parameter before ... and then using it in va_start is undefined behavior — MSVC rejects it with an error. Passing by value avoids this.
| #ifdef CATCH_PLATFORM_MAC | ||
|
|
||
| #define CATCH_TRAP() __asm__("int $3\n" : : ) /* NOLINT */ | ||
| #if defined(__aarch64__) |
There was a problem hiding this comment.
We should look into upgrading catch :-)
There was a problem hiding this comment.
Agreed! The arm64 fix in this PR is just a minimal workaround to unblock macOS arm64 users with the existing single-header Catch v1. Upgrading to Catch2 would be a cleaner long-term solution — happy to do that in a follow-up PR if you'd like.
vcpkg.json
Outdated
| @@ -0,0 +1,7 @@ | |||
| { | |||
| "name": "karambola", | |||
There was a problem hiding this comment.
This seems VC specific? Can we remove it?
There was a problem hiding this comment.
Removed. I used vckpkg to install GSL but this did not have to be part of the PR.
- Replace -D_USE_MATH_DEFINES compiler flag with #ifndef M_PI guard in each source file after <math.h>, per reviewer suggestion - Remove vcpkg.json (MSVC-specific, not needed for cross-platform build) - Remove _USE_MATH_DEFINES from CMakeLists.txt (no longer needed) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
Summary
CMakeLists.txt,vcpkg.json) for cross-platform builds_USE_MATH_DEFINESforM_PImkdir()withstd::filesystem::create_directories(), replaceandalternative token with&&, add missing<sstream>include, fix arm64 debug trap in CatchTested on
Test plan
cmake -B build && cmake --build buildor existingmakectest --test-dir buildormake test🤖 Generated with Claude Code