From c047f96d690cb9cdc844a3c42065e8ecc6d6431b Mon Sep 17 00:00:00 2001 From: Jochen Topf Date: Tue, 3 Mar 2026 09:11:29 +0100 Subject: [PATCH 1/2] Revert "Order deleted objects after visible ones in reverse id order" This reverts commit cff8ff428287aaf501ddb0928a479d014dd5fdd9. Turns out this creates some problems. See #403 --- include/osmium/osm/object_comparisons.hpp | 6 ++---- test/t/osm/test_object_comparisons.cpp | 6 ------ 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/include/osmium/osm/object_comparisons.hpp b/include/osmium/osm/object_comparisons.hpp index 96a5ca7e..85434646 100644 --- a/include/osmium/osm/object_comparisons.hpp +++ b/include/osmium/osm/object_comparisons.hpp @@ -160,11 +160,9 @@ namespace osmium { bool operator()(const osmium::OSMObject& lhs, const osmium::OSMObject& rhs) const noexcept { return const_tie(lhs.type(), lhs.id() > 0, lhs.positive_id(), rhs.version(), - ((lhs.timestamp().valid() && rhs.timestamp().valid()) ? rhs.timestamp() : osmium::Timestamp()), - rhs.visible()) < + ((lhs.timestamp().valid() && rhs.timestamp().valid()) ? rhs.timestamp() : osmium::Timestamp())) < const_tie(rhs.type(), rhs.id() > 0, rhs.positive_id(), lhs.version(), - ((lhs.timestamp().valid() && rhs.timestamp().valid()) ? lhs.timestamp() : osmium::Timestamp()), - lhs.visible()); + ((lhs.timestamp().valid() && rhs.timestamp().valid()) ? lhs.timestamp() : osmium::Timestamp())); } /// @pre lhs and rhs must not be nullptr diff --git a/test/t/osm/test_object_comparisons.cpp b/test/t/osm/test_object_comparisons.cpp index 2ff41276..2254ce41 100644 --- a/test/t/osm/test_object_comparisons.cpp +++ b/test/t/osm/test_object_comparisons.cpp @@ -195,12 +195,6 @@ TEST_CASE("Node comparisons") { REQUIRE(std::is_sorted(nodes.cbegin(), nodes.cend(), osmium::object_order_type_id_reverse_version{})); } - SECTION("reverse version ordering should order objects with deleted flag last") { - nodes.emplace_back(buffer.get(osmium::builder::add_node(buffer, _id( 1), _version(2), _timestamp("2016-01-01T00:00:00Z"), _deleted(false)))); - nodes.emplace_back(buffer.get(osmium::builder::add_node(buffer, _id( 1), _version(2), _timestamp("2016-01-01T00:00:00Z"), _deleted(true)))); - - REQUIRE(std::is_sorted(nodes.cbegin(), nodes.cend(), osmium::object_order_type_id_reverse_version{})); - } } TEST_CASE("Object comparisons: types are ordered nodes, then ways, then relations") { From 3b8595faf1857b94578f2143ad50f10d76e78717 Mon Sep 17 00:00:00 2001 From: Jochen Topf Date: Tue, 3 Mar 2026 10:15:23 +0100 Subject: [PATCH 2/2] Clarify documentation around ordering and add test --- include/osmium/osm/object_comparisons.hpp | 8 +++++--- test/t/osm/test_object_comparisons.cpp | 13 +++++++++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/include/osmium/osm/object_comparisons.hpp b/include/osmium/osm/object_comparisons.hpp index 85434646..a24e53d2 100644 --- a/include/osmium/osm/object_comparisons.hpp +++ b/include/osmium/osm/object_comparisons.hpp @@ -154,15 +154,17 @@ namespace osmium { * (negative IDs first, then positive IDs, both in the order of their * absolute values), but later versions of an object are ordered before * earlier versions of the same object. This is useful when the last - * version of an object needs to be used. + * version of an object needs to be used. Object visibility is not taken + * into account, so objects with different visibilities keep their order. */ struct object_order_type_id_reverse_version { bool operator()(const osmium::OSMObject& lhs, const osmium::OSMObject& rhs) const noexcept { + const bool tsvalid = lhs.timestamp().valid() && rhs.timestamp().valid(); return const_tie(lhs.type(), lhs.id() > 0, lhs.positive_id(), rhs.version(), - ((lhs.timestamp().valid() && rhs.timestamp().valid()) ? rhs.timestamp() : osmium::Timestamp())) < + (tsvalid ? rhs.timestamp() : osmium::Timestamp())) < const_tie(rhs.type(), rhs.id() > 0, rhs.positive_id(), lhs.version(), - ((lhs.timestamp().valid() && rhs.timestamp().valid()) ? lhs.timestamp() : osmium::Timestamp())); + (tsvalid ? lhs.timestamp() : osmium::Timestamp())); } /// @pre lhs and rhs must not be nullptr diff --git a/test/t/osm/test_object_comparisons.cpp b/test/t/osm/test_object_comparisons.cpp index 2254ce41..a8d011d9 100644 --- a/test/t/osm/test_object_comparisons.cpp +++ b/test/t/osm/test_object_comparisons.cpp @@ -195,6 +195,19 @@ TEST_CASE("Node comparisons") { REQUIRE(std::is_sorted(nodes.cbegin(), nodes.cend(), osmium::object_order_type_id_reverse_version{})); } + SECTION("reverse version ordering should keep order for deleted objects") { + nodes.emplace_back(buffer.get(osmium::builder::add_node(buffer, _id( 1), _version(2), _timestamp("2016-01-01T00:00:00Z"), _deleted(false)))); + nodes.emplace_back(buffer.get(osmium::builder::add_node(buffer, _id( 1), _version(2), _timestamp("2016-01-01T00:00:00Z"), _deleted(true)))); + + REQUIRE(std::is_sorted(nodes.cbegin(), nodes.cend(), osmium::object_order_type_id_reverse_version{})); + + nodes.clear(); + + nodes.emplace_back(buffer.get(osmium::builder::add_node(buffer, _id( 1), _version(2), _timestamp("2016-01-01T00:00:00Z"), _deleted(true)))); + nodes.emplace_back(buffer.get(osmium::builder::add_node(buffer, _id( 1), _version(2), _timestamp("2016-01-01T00:00:00Z"), _deleted(false)))); + + REQUIRE(std::is_sorted(nodes.cbegin(), nodes.cend(), osmium::object_order_type_id_reverse_version{})); + } } TEST_CASE("Object comparisons: types are ordered nodes, then ways, then relations") {