From 51890e619bd10069af901cfa6002dbdc3c29940c Mon Sep 17 00:00:00 2001 From: David Eustis Date: Tue, 12 May 2026 19:34:13 +0000 Subject: [PATCH 1/3] pybind: Address review comments on S2CellId bindings - Add S2CellId::kMaxPosition constant (2^61 - 1) to s2cell_id.h and add C++ unit test verifying valid/overflow behavior - Validate pos argument in from_face_pos_level; expose MAX_POSITION as a Python class attribute; add Python test for pos out of range - Switch constants to cls.attr() pattern; update README to document this style - Fix BUILD: add @abseil-cpp//absl/strings dep to s2cell_id_bindings --- src/python/BUILD.bazel | 1 + src/python/README.md | 2 +- src/python/s2cell_id_bindings.cc | 23 +++++++++++++++-------- src/python/s2cell_id_test.py | 4 ++++ src/s2/s2cell_id.h | 1 + src/s2/s2cell_id_test.cc | 20 ++++++++++++++++++++ 6 files changed, 42 insertions(+), 9 deletions(-) diff --git a/src/python/BUILD.bazel b/src/python/BUILD.bazel index a0d83669..89238dbc 100644 --- a/src/python/BUILD.bazel +++ b/src/python/BUILD.bazel @@ -100,6 +100,7 @@ pybind_library( srcs = ["s2cell_id_bindings.cc"], deps = [ "//:s2", + "@abseil-cpp//absl/strings", ], ) diff --git a/src/python/README.md b/src/python/README.md index ee62dbbc..9760f2ac 100644 --- a/src/python/README.md +++ b/src/python/README.md @@ -144,7 +144,7 @@ To add bindings for a new class: Use the following sections to organize functions within the bindings files and tests. Secondarily, follow the order in which functions are declared in the C++ headers. 1. **Constructors** - Default constructors and constructors with parameters -1. **Constants** - Class-level constants in upper snake case (e.g., `S2CellId.MAX_LEVEL`, `S2CellId.NUM_FACES`) +1. **Constants** - Class-level constants in upper snake case (e.g., `S2CellId.MAX_LEVEL`, `S2CellId.NUM_FACES`). Accessible as class attributes. 1. **Factory methods** - Static factory methods (e.g., `from_degrees`, `from_radians`, `zero`, `invalid`) 1. **Properties** - Mutable and read-only properties (e.g., coordinate accessors like `x`, `y`, `lo`, `hi`) 1. **Predicates** - Simple boolean state checks (e.g., `is_empty`, `is_valid`, `is_full`) diff --git a/src/python/s2cell_id_bindings.cc b/src/python/s2cell_id_bindings.cc index 6371a40f..b38ebfda 100644 --- a/src/python/s2cell_id_bindings.cc +++ b/src/python/s2cell_id_bindings.cc @@ -60,10 +60,18 @@ void MaybeThrowChildPositionOutOfRange(int position) { } } +void MaybeThrowPositionOutOfRange(uint64_t pos) { + if (pos > S2CellId::kMaxPosition) { + throw py::value_error( + absl::StrCat("pos ", pos, " out of range [0, ", S2CellId::kMaxPosition, + "]")); + } +} + } // namespace void bind_s2cell_id(py::module& m) { - py::class_(m, "S2CellId", + auto cls = py::class_(m, "S2CellId", "A 64-bit unsigned integer that uniquely identifies a cell in the\n" "S2 cell decomposition.\n\n" "See s2/s2cell_id.h for comprehensive documentation.") @@ -88,12 +96,6 @@ void bind_s2cell_id(py::module& m) { py::arg("latlng"), "Construct a leaf cell containing the given S2LatLng.") - // Constants - .def_readonly_static("MAX_LEVEL", &S2CellId::kMaxLevel, - "Maximum cell subdivision level (30)") - .def_readonly_static("NUM_FACES", &S2CellId::kNumFaces, - "Number of cube faces (6)") - // Factory methods .def_static("from_face", [](int face) { MaybeThrowFaceOutOfRange(face); @@ -103,12 +105,13 @@ void bind_s2cell_id(py::module& m) { "Raises ValueError if face is out of range.") .def_static("from_face_pos_level", [](int face, uint64_t pos, int level) { MaybeThrowFaceOutOfRange(face); + MaybeThrowPositionOutOfRange(pos); MaybeThrowLevelOutOfRange(level, 0, S2CellId::kMaxLevel); return S2CellId::FromFacePosLevel(face, pos, level); }, py::arg("face"), py::arg("pos"), py::arg("level"), "Return a cell given its face, Hilbert curve position, and level.\n\n" - "Raises ValueError if face or level is out of range.") + "Raises ValueError if face, pos, or level is out of range.") .def_static("from_token", [](const std::string& token) { S2CellId cell = S2CellId::FromToken(token); if (cell == S2CellId::None()) { @@ -272,4 +275,8 @@ void bind_s2cell_id(py::module& m) { oss << id; return oss.str(); }); + + cls.attr("MAX_LEVEL") = S2CellId::kMaxLevel; + cls.attr("MAX_POSITION") = S2CellId::kMaxPosition; + cls.attr("NUM_FACES") = S2CellId::kNumFaces; } diff --git a/src/python/s2cell_id_test.py b/src/python/s2cell_id_test.py index caf9b3c9..3c31ea2a 100644 --- a/src/python/s2cell_id_test.py +++ b/src/python/s2cell_id_test.py @@ -57,6 +57,10 @@ def test_from_face_pos_level(self): self.assertEqual(cell.level, 0) self.assertEqual(cell.face, 0) + def test_from_face_pos_level_pos_out_of_range_raises(self): + with self.assertRaises(ValueError): + s2.S2CellId.from_face_pos_level(0, s2.S2CellId.MAX_POSITION + 1, 0) + def test_from_token_roundtrip(self): cell = s2.S2CellId.from_face(3) token = cell.to_token() diff --git a/src/s2/s2cell_id.h b/src/s2/s2cell_id.h index 5045eace..5bf323f7 100644 --- a/src/s2/s2cell_id.h +++ b/src/s2/s2cell_id.h @@ -99,6 +99,7 @@ class S2CellId { static constexpr int kMaxLevel = S2::kMaxCellLevel; // Valid levels: 0..kMaxLevel static constexpr int kPosBits = 2 * kMaxLevel + 1; + static constexpr uint64_t kMaxPosition = (~uint64_t{0}) >> kFaceBits; static constexpr int kMaxSize = 1 << kMaxLevel; explicit constexpr S2CellId(uint64_t id) : id_(id) {} diff --git a/src/s2/s2cell_id_test.cc b/src/s2/s2cell_id_test.cc index 9b3fc48b..0160eeed 100644 --- a/src/s2/s2cell_id_test.cc +++ b/src/s2/s2cell_id_test.cc @@ -101,6 +101,26 @@ TEST(S2CellId, FromFace) { } } +TEST(S2CellId, MaxPosition) { + // kMaxPosition is the largest value that fits in the position field. + EXPECT_EQ(S2CellId::kMaxPosition, (~uint64_t{0}) >> S2CellId::kFaceBits); + + // FromFacePosLevel with kMaxPosition produces a valid cell on the correct face. + for (int face = 0; face < S2CellId::kNumFaces; ++face) { + S2CellId id = S2CellId::FromFacePosLevel(face, S2CellId::kMaxPosition, 0); + EXPECT_TRUE(id.is_valid()); + EXPECT_EQ(face, id.face()); + } + + // pos values above kMaxPosition overflow the position field into the face + // bits, producing an invalid cell. Construct the raw id directly rather + // than via FromFacePosLevel to avoid triggering its is_valid() DCHECK. + constexpr uint64_t kOverflowPos = S2CellId::kMaxPosition + 1; + S2CellId overflow_id( + (static_cast(5) << S2CellId::kPosBits) + (kOverflowPos | 1)); + EXPECT_FALSE(overflow_id.is_valid()); +} + TEST(S2CellId, ParentChildRelationships) { S2CellId id = S2CellId::FromFacePosLevel(3, 0x12345678, S2CellId::kMaxLevel - 4); From fb7a46500e5be9ae7040d4eac2f7a0c1bb89e442 Mon Sep 17 00:00:00 2001 From: David Eustis Date: Tue, 12 May 2026 22:52:46 +0000 Subject: [PATCH 2/3] s2cell_id_test: Improve MaxPosition test comment --- src/s2/s2cell_id_test.cc | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/s2/s2cell_id_test.cc b/src/s2/s2cell_id_test.cc index 0160eeed..6247aa67 100644 --- a/src/s2/s2cell_id_test.cc +++ b/src/s2/s2cell_id_test.cc @@ -113,11 +113,14 @@ TEST(S2CellId, MaxPosition) { } // pos values above kMaxPosition overflow the position field into the face - // bits, producing an invalid cell. Construct the raw id directly rather - // than via FromFacePosLevel to avoid triggering its is_valid() DCHECK. + // bits, producing an invalid cell. + // + // Replicate FromFacePosLevel to avoid its is_valid() DCHECK: + // S2CellId cell((static_cast(face) << kPosBits) + (pos | 1)); + constexpr int face = 5; constexpr uint64_t kOverflowPos = S2CellId::kMaxPosition + 1; S2CellId overflow_id( - (static_cast(5) << S2CellId::kPosBits) + (kOverflowPos | 1)); + (static_cast(face) << S2CellId::kPosBits) + (kOverflowPos | 1)); EXPECT_FALSE(overflow_id.is_valid()); } From 32b3ab89f73f0e25037382d15371fface3021b7f Mon Sep 17 00:00:00 2001 From: David Eustis Date: Wed, 20 May 2026 23:49:16 +0000 Subject: [PATCH 3/3] s2cell_id: Address review comments on kMaxPosition - Use static_cast instead of uint64_t{0} brace-init in the kMaxPosition definition to avoid confusing SWIG - Split MaxPosition test into MaxPositionIsValid and PositionAboveMaxIsInvalid - Use uint64_t{face} in the overflow test per style suggestion --- src/s2/s2cell_id.h | 3 ++- src/s2/s2cell_id_test.cc | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/s2/s2cell_id.h b/src/s2/s2cell_id.h index 5bf323f7..e8f5f952 100644 --- a/src/s2/s2cell_id.h +++ b/src/s2/s2cell_id.h @@ -99,7 +99,8 @@ class S2CellId { static constexpr int kMaxLevel = S2::kMaxCellLevel; // Valid levels: 0..kMaxLevel static constexpr int kPosBits = 2 * kMaxLevel + 1; - static constexpr uint64_t kMaxPosition = (~uint64_t{0}) >> kFaceBits; + static constexpr uint64_t kMaxPosition = + (~static_cast(0)) >> kFaceBits; static constexpr int kMaxSize = 1 << kMaxLevel; explicit constexpr S2CellId(uint64_t id) : id_(id) {} diff --git a/src/s2/s2cell_id_test.cc b/src/s2/s2cell_id_test.cc index 6247aa67..627335d3 100644 --- a/src/s2/s2cell_id_test.cc +++ b/src/s2/s2cell_id_test.cc @@ -101,7 +101,7 @@ TEST(S2CellId, FromFace) { } } -TEST(S2CellId, MaxPosition) { +TEST(S2CellId, MaxPositionIsValid) { // kMaxPosition is the largest value that fits in the position field. EXPECT_EQ(S2CellId::kMaxPosition, (~uint64_t{0}) >> S2CellId::kFaceBits); @@ -111,7 +111,9 @@ TEST(S2CellId, MaxPosition) { EXPECT_TRUE(id.is_valid()); EXPECT_EQ(face, id.face()); } +} +TEST(S2CellId, PositionAboveMaxIsInvalid) { // pos values above kMaxPosition overflow the position field into the face // bits, producing an invalid cell. // @@ -120,7 +122,7 @@ TEST(S2CellId, MaxPosition) { constexpr int face = 5; constexpr uint64_t kOverflowPos = S2CellId::kMaxPosition + 1; S2CellId overflow_id( - (static_cast(face) << S2CellId::kPosBits) + (kOverflowPos | 1)); + (uint64_t{face} << S2CellId::kPosBits) + (kOverflowPos | 1)); EXPECT_FALSE(overflow_id.is_valid()); }