Skip to content

Conversation

@kkaefer
Copy link
Member

@kkaefer kkaefer commented Sep 18, 2025

No description provided.

kkaefer and others added 4 commits September 18, 2025 14:52
- Add googletest as git submodule in vendor/googletest
- Replace TAP-based test infrastructure with googletest
- Convert area tests to parameterized googletest format
- Remove old tap.cpp and tap.hpp files
- Update CMakeLists.txt to build with googletest instead of TAP

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
- Add Google Benchmark as git submodule in vendor/benchmark
- Replace custom timing infrastructure with Google Benchmark
- Convert benchmark functions to use benchmark::State API
- Create separate benchmarks for Earcut and Libtess2 algorithms
- Add proper benchmark registration with meaningful names
- Use benchmark::DoNotOptimize() to prevent result optimization
- Add .claude directory to .gitignore

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Add -ffp-contract=off compiler flag to test targets to ensure consistent
floating-point behavior across different CPU architectures. This prevents
fused multiply-add (FMA) instructions from causing precision differences
that lead to different triangulation results between x86_64 and arm64.

The fix is applied only to test-related targets (fixtures library and
tests executable) and does not affect library users or production builds.

Resolves the issue where issue142 test produced 7 triangles on arm64
instead of the expected 4 triangles that pass on x86_64.

References: GitHub issues #97 and #103, MySQL bug #82760

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
kkaefer and others added 3 commits September 18, 2025 15:29
Add a dedicated Coverage job that:
- Uses g++ with --coverage flag for comprehensive coverage collection
- Installs and configures lcov for coverage data processing
- Filters coverage to focus on main library headers (include/*)
- Excludes system headers, vendor files, tests, and build artifacts
- Generates HTML coverage reports with titles and legends
- Uploads results to Codecov for PR comments and online tracking
- Provides downloadable coverage artifacts as backup

The coverage job runs independently of main builds to avoid performance
overhead while providing detailed coverage metrics and PR feedback.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
- Add new Sanitizers job, testing with combined ASan+UBSan
- Use clang++ compiler for better sanitizer support
- Configure strict sanitizer options to abort on any detected issues
- Enables detection of memory leaks, buffer overflows, and undefined behavior

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
- Check all C++ files (*.hpp, *.cpp) in include/ and test/ directories
- Fail CI if code is not formatted according to clang-format style
- Generate and upload formatting patch as artifact when issues found
- Show patch content in CI logs for easy review
- Helps maintain consistent code style across the project

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@kkaefer kkaefer requested a review from Copilot September 18, 2025 14:01
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

A code formatting update that reformats test fixture data and migrates from a custom TAP testing framework to Google Test (gtest). The formatting changes improve readability by properly spacing coordinate arrays and constructor parameters across multiple lines.

  • Migrated test framework from custom TAP to Google Test with proper test parametrization
  • Reformatted large coordinate arrays in fixture files for better readability
  • Updated code formatting throughout visualization and test files

Reviewed Changes

Copilot reviewed 60 out of 66 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test/viz.cpp Code formatting improvements including include reordering and bracket style updates
test/test.cpp Complete migration from TAP framework to Google Test with parametrized tests
test/tap.hpp Removed custom TAP testing framework header
test/tap.cpp Removed custom TAP testing framework implementation
test/fixtures/*.cpp Reformatted coordinate arrays and constructor parameters for better readability

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +1 to +8
#include <gtest/gtest.h>

#include <algorithm>
#include <iomanip>
#include <locale>
#include <sstream>

#include "fixtures/geometries.hpp"
Copy link

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider grouping system/standard library includes before project-specific includes for better organization and to follow common C++ conventions.

Copilot uses AI. Check for mistakes.
Comment on lines +105 to +113
INSTANTIATE_TEST_SUITE_P(FixtureTests,
EarcutAreaTest,
::testing::ValuesIn(mapbox::fixtures::FixtureTester::collection()),
[](const ::testing::TestParamInfo<mapbox::fixtures::FixtureTester*>& info) {
std::string name = info.param->name;
std::replace(name.begin(), name.end(), '-', '_');
std::replace(name.begin(), name.end(), '.', '_');
return name;
});
Copy link

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

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

[nitpick] The lambda function for test name transformation should be extracted to a separate function to improve readability and potentially allow reuse if similar test instantiations are needed elsewhere.

Suggested change
INSTANTIATE_TEST_SUITE_P(FixtureTests,
EarcutAreaTest,
::testing::ValuesIn(mapbox::fixtures::FixtureTester::collection()),
[](const ::testing::TestParamInfo<mapbox::fixtures::FixtureTester*>& info) {
std::string name = info.param->name;
std::replace(name.begin(), name.end(), '-', '_');
std::replace(name.begin(), name.end(), '.', '_');
return name;
});
// Extracted function for test name transformation
std::string TransformTestName(const ::testing::TestParamInfo<mapbox::fixtures::FixtureTester*>& info) {
std::string name = info.param->name;
std::replace(name.begin(), name.end(), '-', '_');
std::replace(name.begin(), name.end(), '.', '_');
return name;
}
INSTANTIATE_TEST_SUITE_P(FixtureTests,
EarcutAreaTest,
::testing::ValuesIn(mapbox::fixtures::FixtureTester::collection()),
TransformTestName);

Copilot uses AI. Check for mistakes.
Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Very nice! Maybe let's disable code formatting on the fixtures though, and revert the changes? A bit too much whitespace on those after...

@kkaefer
Copy link
Member Author

kkaefer commented Sep 18, 2025

Maybe let's disable code formatting on the fixtures though, and revert the changes? A bit too much whitespace on those after...

I considered that, but decided to bite the bullet and add it. I think it's much easier for a human to understand the nature of the polygons with those fixes.

@kkaefer kkaefer merged commit f36ced7 into master Sep 18, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants