Skip to content

Commit cff653a

Browse files
committed
Wire -Wall -Wextra -Wpedantic into the actual build
-Wall -Wextra -Wpedantic has been a manual step this session, run by hand from a scratch script against geoarrow_arrays.cpp only, and reported in commit messages as if it were a check the build enforces. It never was, and it never covered geoarrow.cpp or intersections.cpp - the pybind11 layer - at all, since those compile only through the top-level CMakeLists.txt and no manual pass this session reached them. Wiring the flags into both CMakeLists.txt directly means every push gets the check for free, on whichever compiler build.yml already uses per platform (gcc on ubuntu, Apple clang on macos, cl on windows) - without adding a job. Not MSVC, whose warning flags are different syntax; not the vendored nanoarrow.c/geoarrow.c, which are not ours to keep warning-free and which -isystem does not shield when a .c file is compiled directly rather than reached through #include - confirmed by compiling one by hand before writing this. So the vendored sources move into a small vendor_arrow OBJECT library that the flags are never applied to, linked into run_tests/run_benchmarks/intersections via $<TARGET_OBJECTS:>, which changes nothing about what ships. Building the pybind11 layer with the flags on for the first time found nothing in it. What it did surface, unrelated to any of this and left alone: utils.hpp's Exception::what() hides std::exception's virtual under clang (-Woverloaded-virtual - the class is thrown by value at its only call site and never caught as an exception, so the override is dead code, not a live bug), and three pre-existing sign-compare warnings plus one unused-function warning in tests_intersections.cpp under both compilers. Not touched here. No -Werror: that would fail the build on those two pre-existing, unrelated warnings today. This makes new warnings visible in every build log; turning that into a hard gate is a separate decision once the existing ones are triaged. Verified: a full extension/CMakeLists.txt rebuild (run_tests, 36,085 assertions; run_benchmarks) and a direct cmake configure+build of the top-level intersections module against this checkout's Python and pybind11, both clean under the new flags apart from the two warnings above; the resulting module passes the full 137-test Python suite; a real `pip install .` still succeeds through scikit-build-core. The matching CI wiring (building run_benchmarks in build.yml, and a workflow_dispatch trigger on package.yml for on-demand cibuildwheel runs) is not in this commit: this session's GitHub token lacks the workflow scope GitHub requires for changes under .github/workflows/, so that push was rejected. The exact diff is given separately for manual application. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SqdWshmD4AUHhrqeMh86GS
1 parent 816dafa commit cff653a

2 files changed

Lines changed: 35 additions & 6 deletions

File tree

CMakeLists.txt

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,15 +22,27 @@ set(GEOARROW_DIR ${VENDOR_DIR}/geoarrow)
2222
find_package(Python REQUIRED COMPONENTS Interpreter Development.Module)
2323
find_package(pybind11 CONFIG REQUIRED)
2424

25+
# The warnings we hold our own code to. Not MSVC - the flags are GCC/Clang
26+
# syntax - and not the vendored C below: -isystem only quiets warnings from
27+
# a header reached through it, not from a .c file compiled directly out of
28+
# that directory, so the vendored sources are kept in a target of their own
29+
# that this is never applied to.
30+
if(NOT MSVC)
31+
set(SNAIL_WARNING_FLAGS -Wall -Wextra -Wpedantic)
32+
endif()
33+
34+
add_library(vendor_arrow OBJECT ${NANOARROW_DIR}/nanoarrow.c ${GEOARROW_DIR}/geoarrow.c)
35+
set_target_properties(vendor_arrow PROPERTIES C_STANDARD 99 C_STANDARD_REQUIRED ON)
36+
target_include_directories(vendor_arrow PRIVATE ${VENDOR_DIR} ${NANOARROW_DIR})
37+
2538
python_add_library(intersections MODULE ${CPP_SRC_DIR}/intersections.cpp WITH_SOABI
2639
${CPP_SRC_DIR}/geoarrow.cpp ${CPP_SRC_DIR}/geoarrow_arrays.cpp
2740
${CPP_SRC_DIR}/geometry.cpp
2841
${CPP_SRC_DIR}/operations.cpp
29-
${NANOARROW_DIR}/nanoarrow.c
30-
${GEOARROW_DIR}/geoarrow.c)
42+
$<TARGET_OBJECTS:vendor_arrow>)
3143

3244
target_compile_features(intersections PRIVATE cxx_std_17)
33-
set_target_properties(intersections PROPERTIES C_STANDARD 99 C_STANDARD_REQUIRED ON)
45+
target_compile_options(intersections PRIVATE ${SNAIL_WARNING_FLAGS})
3446
target_include_directories(intersections PUBLIC ${CPP_SRC_DIR})
3547
# SYSTEM: vendored code is not ours to keep warning-free
3648
target_include_directories(intersections SYSTEM PRIVATE ${VENDOR_DIR} ${NANOARROW_DIR})

extension/CMakeLists.txt

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,24 @@ set(CPP_TESTS_DIR tests)
1414
# not while that code sat beside the bindings.
1515
set(VENDOR_DIR vendor)
1616

17+
# The warnings we hold our own code to. Not MSVC - the flags are GCC/Clang
18+
# syntax - and not the vendored C below: -isystem only quiets warnings from
19+
# a header reached through it, not from a .c file compiled directly out of
20+
# that directory, so the vendored sources are kept in a target of their own
21+
# that this is never applied to.
22+
if(NOT MSVC)
23+
set(SNAIL_WARNING_FLAGS -Wall -Wextra -Wpedantic)
24+
endif()
25+
1726
add_subdirectory(extern/Catch2)
27+
28+
add_library(vendor_arrow OBJECT
29+
${VENDOR_DIR}/nanoarrow/nanoarrow.c
30+
${VENDOR_DIR}/geoarrow/geoarrow.c
31+
)
32+
set_target_properties(vendor_arrow PROPERTIES C_STANDARD 99 C_STANDARD_REQUIRED ON)
33+
target_include_directories(vendor_arrow PRIVATE ${VENDOR_DIR} ${VENDOR_DIR}/nanoarrow)
34+
1835
add_executable(run_tests
1936
${CPP_TESTS_DIR}/run_tests.cpp
2037
${CPP_TESTS_DIR}/tests_intersections.cpp
@@ -24,10 +41,9 @@ add_executable(run_tests
2441
${CPP_SRC_DIR}/geoarrow_arrays.cpp
2542
${CPP_SRC_DIR}/geometry.cpp
2643
${CPP_SRC_DIR}/operations.cpp
27-
${VENDOR_DIR}/nanoarrow/nanoarrow.c
28-
${VENDOR_DIR}/geoarrow/geoarrow.c
44+
$<TARGET_OBJECTS:vendor_arrow>
2945
)
30-
set_target_properties(run_tests PROPERTIES C_STANDARD 99 C_STANDARD_REQUIRED ON)
46+
target_compile_options(run_tests PRIVATE ${SNAIL_WARNING_FLAGS})
3147
target_include_directories(run_tests PUBLIC ${CPP_SRC_DIR})
3248
# SYSTEM: vendored code is not ours to keep warning-free
3349
target_include_directories(run_tests SYSTEM PRIVATE
@@ -38,6 +54,7 @@ add_executable(run_benchmarks
3854
benchmarks/benchmark_split.cpp
3955
${CPP_SRC_DIR}/operations.cpp
4056
)
57+
target_compile_options(run_benchmarks PRIVATE ${SNAIL_WARNING_FLAGS})
4158
target_include_directories(run_benchmarks PUBLIC ${CPP_SRC_DIR})
4259
if(NOT MSVC)
4360
target_compile_options(run_benchmarks PRIVATE -O2)

0 commit comments

Comments
 (0)