feat: Add ShapeIndex overload for s2_boolean_operation() (#51)

It might be a good idea to add this for other ShapeIndexGeography functions as well since we now have the EncodedShapeIndexGeography as well. Also makes compiler warnings opt-in for now since they cause problems on conda.
paleolimbot · Nov 23, 2024 · 5b06341 · 5b06341
1 parent a1ce3d0
commit 5b06341
Show file tree

Hide file tree

Showing 4 changed files with 43 additions and 27 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -23,6 +23,11 @@ set(CMAKE_POSITION_INDEPENDENT_CODE ON)
 list(APPEND CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake/modules")
 
 option(S2GEOGRAPHY_BUILD_TESTS "Build tests" OFF)
+# These are opt-in because it's hard for all users to control the warnings from
+# s2 and Abseil on all platforms. Even when turned on, the extra warnings are
+# only added in debug mode.
+option(S2GEOGRAPHY_EXTRA_WARNINGS
+       "Enable extra compiler warnings in debug mode" OFF)
 option(S2GEOGRAPHY_CODE_COVERAGE "Enable coverage reporting" OFF)
 option(S2GEOGRAPHY_BUILD_EXAMPLES "Build s2geography examples" OFF)
 option(BUILD_SHARED_LIBS "Build using shared libraries" ON)
@@ -266,29 +271,31 @@ endif()
 target_link_libraries(s2geography PUBLIC s2::s2 absl::memory absl::str_format)
 
 # Set somewhat aggressive compiler warning flags
-if(CMAKE_C_COMPILER_ID STREQUAL "GNU")
-  target_compile_options(
-    s2geography
-    PRIVATE $<$<CONFIG:Debug>:-Wall
-            -Werror
-            -Wextra
-            -Wpedantic
-            -Wno-type-limits
-            -Wmaybe-uninitialized
-            -Wunused-result
-            -Wconversion
-            -Wno-sign-conversion>)
-elseif(CMAKE_C_COMPILER_ID STREQUAL "AppleClang" OR CMAKE_C_COMPILER_ID
-                                                    STREQUAL "Clang")
-  target_compile_options(
-    s2geography
-    PRIVATE $<$<CONFIG:Debug>:-Wall
-            -Werror
-            -Wextra
-            -Wpedantic
-            -Wdocumentation
-            -Wconversion
-            -Wno-sign-conversion>)
+if(S2GEOGRAPHY_EXTRA_WARNINGS)
+  if(CMAKE_C_COMPILER_ID STREQUAL "GNU")
+    target_compile_options(
+      s2geography
+      PRIVATE $<$<CONFIG:Debug>:-Wall
+              -Werror
+              -Wextra
+              -Wpedantic
+              -Wno-type-limits
+              -Wmaybe-uninitialized
+              -Wunused-result
+              -Wconversion
+              -Wno-sign-conversion>)
+  elseif(CMAKE_C_COMPILER_ID STREQUAL "AppleClang" OR CMAKE_C_COMPILER_ID
+                                                      STREQUAL "Clang")
+    target_compile_options(
+      s2geography
+      PRIVATE $<$<CONFIG:Debug>:-Wall
+              -Werror
+              -Wextra
+              -Wpedantic
+              -Wdocumentation
+              -Wconversion
+              -Wno-sign-conversion>)
+  endif()
 endif()
 
 if(S2GEOGRAPHY_BUILD_TESTS)

diff --git a/src/s2geography/build.cc b/src/s2geography/build.cc
@@ -95,7 +95,7 @@ std::unique_ptr<Geography> s2_geography_from_layers(
 }
 
 static std::unique_ptr<Geography> s2_boolean_operation(
-    const ShapeIndexGeography& geog1, const ShapeIndexGeography& geog2,
+    const S2ShapeIndex& geog1, const S2ShapeIndex& geog2,
     S2BooleanOperation::OpType op_type, const GlobalOptions& options,
     GlobalOptions::OutputAction point_layer_action,
     GlobalOptions::OutputAction polyline_layer_action,
@@ -122,7 +122,7 @@ static std::unique_ptr<Geography> s2_boolean_operation(
 
   // do the boolean operation, build layers, and check for errors
   S2Error error;
-  if (!op.Build(geog1.ShapeIndex(), geog2.ShapeIndex(), &error)) {
+  if (!op.Build(geog1, geog2, &error)) {
     throw Exception(error.text());
   }
 
@@ -133,7 +133,7 @@ static std::unique_ptr<Geography> s2_boolean_operation(
 }
 
 std::unique_ptr<Geography> s2_boolean_operation(
-    const ShapeIndexGeography& geog1, const ShapeIndexGeography& geog2,
+    const S2ShapeIndex& geog1, const S2ShapeIndex& geog2,
     S2BooleanOperation::OpType op_type, const GlobalOptions& options) {
   return s2_boolean_operation(
       geog1, geog2, op_type, options, options.point_layer_action,

diff --git a/src/s2geography/build.h b/src/s2geography/build.h
@@ -35,9 +35,17 @@ class GlobalOptions {
 };
 
 std::unique_ptr<Geography> s2_boolean_operation(
-    const ShapeIndexGeography& geog1, const ShapeIndexGeography& geog2,
+    const S2ShapeIndex& geog1, const S2ShapeIndex& geog2,
     S2BooleanOperation::OpType op_type, const GlobalOptions& options);
 
+template <typename IndexGeogLHS, typename IndexGeogRHS>
+std::unique_ptr<Geography> s2_boolean_operation(
+    const IndexGeogLHS& geog1, const IndexGeogRHS& geog2,
+    S2BooleanOperation::OpType op_type, const GlobalOptions& options) {
+  return s2_boolean_operation(geog1.ShapeIndex(), geog2.ShapeIndex(), op_type,
+                              options);
+}
+
 std::unique_ptr<Geography> s2_unary_union(const ShapeIndexGeography& geog,
                                           const GlobalOptions& options);
 

diff --git a/src/s2geography/s2geography_gtest_util.h b/src/s2geography/s2geography_gtest_util.h
@@ -3,6 +3,7 @@
 #include <gmock/gmock-matchers.h>
 #include <gtest/gtest.h>
 
+#include <ostream>
 #include <vector>
 
 #include "s2geography/geography.h"