From 5aad427143884e0d62608c7d7daaa0eb43cee9c6 Mon Sep 17 00:00:00 2001 From: Henry Schreiner Date: Thu, 23 Jul 2026 11:05:49 -0400 Subject: [PATCH] fix: ambiguous comparison between indexed accessor and cell reference Comparing two const accessors, or an accessor with a storage cell reference, did not compile with the default storage: the accessor's catch-all comparison templates and the mirrored friend operators of unlimited_storage::reference were equally good matches. Make the accessor-vs-accessor operators const so that standard algorithms (which compare through const references) use them, and constrain the generic operators in detail/operators.hpp to yield when the other operand's class handles the comparison with a member, as operator== already did. Fixes #403 Assisted-by: ClaudeCode:claude-fable-5 --- include/boost/histogram/detail/detect.hpp | 10 ++++ include/boost/histogram/detail/operators.hpp | 43 +++++++++------ include/boost/histogram/indexed.hpp | 12 ++--- test/indexed_test.cpp | 57 ++++++++++++++++++++ 4 files changed, 100 insertions(+), 22 deletions(-) diff --git a/include/boost/histogram/detail/detect.hpp b/include/boost/histogram/detail/detect.hpp index d62ab4ddd..f08b79f46 100644 --- a/include/boost/histogram/detail/detect.hpp +++ b/include/boost/histogram/detail/detect.hpp @@ -111,6 +111,16 @@ BOOST_HISTOGRAM_DETAIL_DETECT_BINARY(has_operator_rdiv, (t /= u)); BOOST_HISTOGRAM_DETAIL_DETECT_BINARY(has_method_eq, (cref().operator==(u))); +BOOST_HISTOGRAM_DETAIL_DETECT_BINARY(has_method_ne, (cref().operator!=(u))); + +BOOST_HISTOGRAM_DETAIL_DETECT_BINARY(has_method_lt, (cref().operator<(u))); + +BOOST_HISTOGRAM_DETAIL_DETECT_BINARY(has_method_gt, (cref().operator>(u))); + +BOOST_HISTOGRAM_DETAIL_DETECT_BINARY(has_method_le, (cref().operator<=(u))); + +BOOST_HISTOGRAM_DETAIL_DETECT_BINARY(has_method_ge, (cref().operator>=(u))); + BOOST_HISTOGRAM_DETAIL_DETECT(has_threading_support, (T::has_threading_support)); // stronger form of std::is_convertible that works with explicit operator T and ctors diff --git a/include/boost/histogram/detail/operators.hpp b/include/boost/histogram/detail/operators.hpp index dead8902f..16a54780a 100644 --- a/include/boost/histogram/detail/operators.hpp +++ b/include/boost/histogram/detail/operators.hpp @@ -20,10 +20,11 @@ namespace detail { template using if_not_same = std::enable_if_t<(!std::is_same::value), bool>; -// template -// using if_not_same_and_has_eq = -// std::enable_if_t<(!std::is_same::value && !has_method_eq::value), -// bool>; +// A generic template operator must yield to U's own member operator when U has one that +// accepts T, otherwise the two catch-all templates are ambiguous (issue #403). +template +using if_not_same_and_no_method = + std::enable_if_t<(!std::is_same::value && !HasMethod::value), bool>; // totally_ordered is for types with a <= b == !(a > b) [floats with NaN violate this] // Derived must implement <,== for symmetric form and <,>,== for non-symmetric. @@ -44,11 +45,13 @@ struct mirrored { template struct mirrored { template - friend if_not_same operator<(const U& a, const T& b) noexcept { + friend if_not_same_and_no_method> operator<( + const U& a, const T& b) noexcept { return b > a; } template - friend if_not_same operator>(const U& a, const T& b) noexcept { + friend if_not_same_and_no_method> operator>( + const U& a, const T& b) noexcept { return b < a; } template @@ -57,15 +60,18 @@ struct mirrored { return b.operator==(a); } template - friend if_not_same operator<=(const U& a, const T& b) noexcept { + friend if_not_same_and_no_method> operator<=( + const U& a, const T& b) noexcept { return b >= a; } template - friend if_not_same operator>=(const U& a, const T& b) noexcept { + friend if_not_same_and_no_method> operator>=( + const U& a, const T& b) noexcept { return b <= a; } template - friend if_not_same operator!=(const U& a, const T& b) noexcept { + friend if_not_same_and_no_method> operator!=( + const U& a, const T& b) noexcept { return b != a; } }; @@ -83,7 +89,8 @@ struct equality { template struct equality { template - friend if_not_same operator!=(const T& a, const U& b) noexcept { + friend if_not_same_and_no_method> operator!=( + const T& a, const U& b) noexcept { return !(a == b); } }; @@ -97,18 +104,20 @@ struct totally_ordered_impl : equality, mirrored { template struct totally_ordered_impl : equality, mirrored { template - friend if_not_same operator<=(const T& a, const U& b) noexcept { + friend if_not_same_and_no_method> operator<=( + const T& a, const U& b) noexcept { return !(a > b); } template - friend if_not_same operator>=(const T& a, const U& b) noexcept { + friend if_not_same_and_no_method> operator>=( + const T& a, const U& b) noexcept { return !(a < b); } }; template using totally_ordered = mp11::mp_rename< - mp11::mp_product, mp11::mp_list >, + mp11::mp_product, mp11::mp_list>, mp11::mp_inherit>; template @@ -120,18 +129,20 @@ struct partially_ordered_impl : equality, mirrored { template struct partially_ordered_impl : equality, mirrored { template - friend if_not_same operator<=(const T& a, const U& b) noexcept { + friend if_not_same_and_no_method> operator<=( + const T& a, const U& b) noexcept { return a < b || a == b; } template - friend if_not_same operator>=(const T& a, const U& b) noexcept { + friend if_not_same_and_no_method> operator>=( + const T& a, const U& b) noexcept { return a > b || a == b; } }; template using partially_ordered = mp11::mp_rename< - mp11::mp_product, mp11::mp_list >, + mp11::mp_product, mp11::mp_list>, mp11::mp_inherit>; } // namespace detail diff --git a/include/boost/histogram/indexed.hpp b/include/boost/histogram/indexed.hpp index 66690e600..ccf92345d 100644 --- a/include/boost/histogram/indexed.hpp +++ b/include/boost/histogram/indexed.hpp @@ -179,12 +179,12 @@ class BOOST_ATTRIBUTE_NODISCARD indexed_range { } // forward all comparison operators to the value - bool operator<(const accessor& o) noexcept { return get() < o.get(); } - bool operator>(const accessor& o) noexcept { return get() > o.get(); } - bool operator==(const accessor& o) noexcept { return get() == o.get(); } - bool operator!=(const accessor& o) noexcept { return get() != o.get(); } - bool operator<=(const accessor& o) noexcept { return get() <= o.get(); } - bool operator>=(const accessor& o) noexcept { return get() >= o.get(); } + bool operator<(const accessor& o) const noexcept { return get() < o.get(); } + bool operator>(const accessor& o) const noexcept { return get() > o.get(); } + bool operator==(const accessor& o) const noexcept { return get() == o.get(); } + bool operator!=(const accessor& o) const noexcept { return get() != o.get(); } + bool operator<=(const accessor& o) const noexcept { return get() <= o.get(); } + bool operator>=(const accessor& o) const noexcept { return get() >= o.get(); } template bool operator<(const U& o) const noexcept { diff --git a/test/indexed_test.cpp b/test/indexed_test.cpp index 8ddbadff8..4f183d46f 100644 --- a/test/indexed_test.cpp +++ b/test/indexed_test.cpp @@ -156,6 +156,60 @@ void run_stdlib_tests(mp_list) { } } +// regression test for https://github.com/boostorg/histogram/issues/403; with the +// default storage, comparisons of accessors and cell references were ambiguous +template +void run_comparison_tests(Tag) { + auto h = make(Tag(), axis::integer<>(0, 2), axis::integer<>(0, 2)); + h(0, 0); + h(1, 1); + h(1, 1); + + auto ind = indexed(h); + + // libc++ passes accessors as const references to the comparator + auto imin = std::min_element(ind.begin(), ind.end()); + BOOST_TEST_EQ(**imin, 0); + auto imax = std::max_element(ind.begin(), ind.end()); + BOOST_TEST_EQ(**imax, 2); + + auto it = ind.begin(); // points to cell (0, 0) with value 1 + auto&& a = *it; + const auto& ca = a; + auto&& r = h.at(1, 1); // value 2 + + BOOST_TEST(a < r); + BOOST_TEST(a <= r); + BOOST_TEST(a != r); + BOOST_TEST(!(a > r)); + BOOST_TEST(!(a >= r)); + BOOST_TEST(!(a == r)); + + BOOST_TEST(r > a); + BOOST_TEST(r >= a); + BOOST_TEST(r != a); + BOOST_TEST(!(r < a)); + BOOST_TEST(!(r <= a)); + BOOST_TEST(!(r == a)); + + BOOST_TEST(ca == ca); + BOOST_TEST(ca <= ca); + BOOST_TEST(ca >= ca); + BOOST_TEST(!(ca != ca)); + BOOST_TEST(!(ca < ca)); + BOOST_TEST(!(ca > ca)); + BOOST_TEST(ca < r); + + BOOST_TEST(a == 1); + BOOST_TEST(1 == a); + BOOST_TEST(a != 2); + BOOST_TEST(2 != a); + BOOST_TEST(a < 2); + BOOST_TEST(2 > a); + BOOST_TEST(a <= 1); + BOOST_TEST(1 >= a); +} + template void run_indexed_with_range_tests(Tag) { { @@ -201,6 +255,9 @@ int main() { run_stdlib_tests(x); }); + run_comparison_tests(static_tag{}); + run_comparison_tests(dynamic_tag{}); + run_indexed_with_range_tests(static_tag{}); run_indexed_with_range_tests(dynamic_tag{}); return boost::report_errors();