From f46a2c261eb4491e549acf89d3d815bc23e33ea4 Mon Sep 17 00:00:00 2001
From: Barend Gehrels <barend@xs4all.nl>
Date: Wed, 27 Oct 2021 12:46:06 +0200
Subject: [PATCH] [equals] use collected_vector as a type, no specialization
 (review result)

---
 .../detail/equals/collect_vectors.hpp         | 109 ++++++++----------
 .../detail/equals/implementation.hpp          |  38 +++---
 test/algorithms/equals/Jamfile                |   1 +
 test/algorithms/equals/equals_on_spheroid.cpp |   2 -
 test/algorithms/equals/equals_sph.cpp         |   8 ++
 test/algorithms/equals/test_equals.hpp        |  12 +-
 6 files changed, 87 insertions(+), 83 deletions(-)

diff --git a/include/boost/geometry/algorithms/detail/equals/collect_vectors.hpp b/include/boost/geometry/algorithms/detail/equals/collect_vectors.hpp
index 44cab2f23..3abd01cf5 100644
--- a/include/boost/geometry/algorithms/detail/equals/collect_vectors.hpp
+++ b/include/boost/geometry/algorithms/detail/equals/collect_vectors.hpp
@@ -46,26 +46,17 @@
 namespace boost { namespace geometry
 {
 
-template
-<
-    typename T,
-    typename Geometry,
-    typename CSTag = typename cs_tag<Geometry>::type
->
-struct collected_vector
-    : nyi::not_implemented_tag
-{};
 
-template <typename T, typename Geometry>
-struct collected_vector<T, Geometry, cartesian_tag>
+template <typename T>
+struct collected_vector_cartesian
 {
     typedef T type;
 
-    inline collected_vector()
+    inline collected_vector_cartesian()
     {}
 
-    inline collected_vector(T const& px, T const& py,
-                            T const& pdx, T const& pdy)
+    inline collected_vector_cartesian(T const& px, T const& py,
+                                      T const& pdx, T const& pdy)
         : x(px)
         , y(py)
         , dx(pdx)
@@ -75,7 +66,7 @@ struct collected_vector<T, Geometry, cartesian_tag>
     {}
 
     template <typename Point>
-    inline collected_vector(Point const& p1, Point const& p2)
+    inline collected_vector_cartesian(Point const& p1, Point const& p2)
         : x(get<0>(p1))
         , y(get<1>(p1))
         , dx(get<0>(p2) - x)
@@ -100,7 +91,7 @@ struct collected_vector<T, Geometry, cartesian_tag>
     }
 
     // For sorting
-    inline bool operator<(collected_vector const& other) const
+    inline bool operator<(collected_vector_cartesian const& other) const
     {
         if (math::equals(x, other.x))
         {
@@ -117,13 +108,13 @@ struct collected_vector<T, Geometry, cartesian_tag>
         return x < other.x;
     }
 
-    inline bool next_is_collinear(collected_vector const& other) const
+    inline bool next_is_collinear(collected_vector_cartesian const& other) const
     {
         return same_direction(other);
     }
 
     // For std::equals
-    inline bool operator==(collected_vector const& other) const
+    inline bool operator==(collected_vector_cartesian const& other) const
     {
         return math::equals(x, other.x)
             && math::equals(y, other.y)
@@ -131,7 +122,7 @@ struct collected_vector<T, Geometry, cartesian_tag>
     }
 
 private:
-    inline bool same_direction(collected_vector const& other) const
+    inline bool same_direction(collected_vector_cartesian const& other) const
     {
         // For high precision arithmetic, we have to be
         // more relaxed then using ==
@@ -149,35 +140,32 @@ private:
 // Compatible with spherical_side_formula which currently
 // is the default spherical_equatorial and geographic strategy
 // so CSTag is spherical_equatorial_tag or geographic_tag
-template <typename T, typename Geometry>
-struct collected_vector<T, Geometry, spherical_equatorial_tag>
+template <typename T, typename Point>
+struct collected_vector_spherical
 {
     typedef T type;
 
-    typedef typename geometry::detail::cs_angular_units<Geometry>::type units_type;
-    typedef model::point<T, 2, cs::spherical_equatorial<units_type> > point_type;
     typedef model::point<T, 3, cs::cartesian> vector_type;
 
-    collected_vector()
+    collected_vector_spherical()
     {}
 
-    template <typename Point>
-    collected_vector(Point const& p1, Point const& p2)
+    collected_vector_spherical(Point const& p1, Point const& p2)
         : origin(get<0>(p1), get<1>(p1))
     {
-        origin = detail::return_normalized<point_type>(
+        origin = detail::return_normalized<Point>(
                     origin,
                     strategy::normalize::spherical_point());
 
         using namespace geometry::formula;
         prev = sph_to_cart3d<vector_type>(p1);
         next = sph_to_cart3d<vector_type>(p2);
-        direction = cross_product(prev, next);
+        cross = direction = cross_product(prev, next);
     }
 
     bool normalize()
     {
-        T magnitude_sqr = dot_product(direction, direction);
+        T const magnitude_sqr = dot_product(direction, direction);
 
         // NOTE: shouldn't here math::equals() be called?
         if (magnitude_sqr > 0)
@@ -189,7 +177,7 @@ struct collected_vector<T, Geometry, spherical_equatorial_tag>
         return false;
     }
 
-    bool operator<(collected_vector const& other) const
+    bool operator<(collected_vector_spherical const& other) const
     {
         if (math::equals(get<0>(origin), get<0>(other.origin)))
         {
@@ -213,13 +201,13 @@ struct collected_vector<T, Geometry, spherical_equatorial_tag>
     // For consistency with side and intersection strategies used by relops
     // IMPORTANT: this method should be called for previous vector
     // and next vector should be passed as parameter
-    bool next_is_collinear(collected_vector const& other) const
+    bool next_is_collinear(collected_vector_spherical const& other) const
     {
-        return formula::sph_side_value(direction, other.next) == 0;
+        return formula::sph_side_value(cross, other.next) == 0;
     }
 
     // For std::equals
-    bool operator==(collected_vector const& other) const
+    bool operator==(collected_vector_spherical const& other) const
     {
         return math::equals(get<0>(origin), get<0>(other.origin))
             && math::equals(get<1>(origin), get<1>(other.origin))
@@ -228,62 +216,57 @@ struct collected_vector<T, Geometry, spherical_equatorial_tag>
 
 private:
     // For consistency with side and intersection strategies used by relops
-    bool is_collinear(collected_vector const& other) const
+    // NOTE: alternative would be to equal-compare direction's coordinates
+    //       or to check if dot product of directions is equal to 1.
+    bool is_collinear(collected_vector_spherical const& other) const
     {
-        return formula::sph_side_value(direction, other.prev) == 0
-            && formula::sph_side_value(direction, other.next) == 0;
+        return formula::sph_side_value(cross, other.prev) == 0
+            && formula::sph_side_value(cross, other.next) == 0;
     }
-    
-    /*bool same_direction(collected_vector const& other) const
-    {
-        return math::equals_with_epsilon(get<0>(direction), get<0>(other.direction))
-            && math::equals_with_epsilon(get<1>(direction), get<1>(other.direction))
-            && math::equals_with_epsilon(get<2>(direction), get<2>(other.direction));
-    }*/
 
-    point_type origin; // used for sorting and equality check
+    Point origin; // used for sorting and equality check
     vector_type direction; // used for sorting, only in operator<
+    vector_type cross; // used for sorting, used for collinearity check
     vector_type prev; // used for collinearity check, only in operator==
     vector_type next; // used for collinearity check
 };
 
-// Specialization for spherical polar
-template <typename T, typename Geometry>
-struct collected_vector<T, Geometry, spherical_polar_tag>
-    : public collected_vector<T, Geometry, spherical_equatorial_tag>
+// Version for spherical polar
+template <typename T, typename Point>
+struct collected_vector_polar
+    : public collected_vector_spherical<T, Point>
 {
-    typedef T type;
-    using base_type = collected_vector<T, Geometry, spherical_equatorial_tag>;
+    using type = T;
+    using base_point_type
+        = model::point<T, 2, cs::spherical_equatorial<geometry::degree>>;
+    using base_type = collected_vector_spherical<T, base_point_type>;
 
-    collected_vector() {}
+    collected_vector_polar() {}
 
-    template <typename Point>
-    collected_vector(Point const& p1, Point const& p2)
+    collected_vector_polar(Point const& p1, Point const& p2)
         : base_type(to_equatorial(p1), to_equatorial(p2))
     {}
 
 private:
-    template <typename Point>
-    Point to_equatorial(Point const& p)
+    static base_point_type to_equatorial(Point const& p)
     {
-        typedef typename coordinate_type<Point>::type coord_type;
-
-        typedef math::detail::constants_on_spheroid
+        using coord_type = typename coordinate_type<Point>::type;
+        using constants = math::detail::constants_on_spheroid
             <
                 coord_type,
                 typename coordinate_system<Point>::type::units
-            > constants;
+            > ;
 
-        coord_type const pi_2 = constants::half_period() / 2;
+        constexpr coord_type pi_2 = constants::half_period() / 2;
 
-        Point res = p;
+        base_point_type res;
+        set<0>(res, get<0>(p));
         set<1>(res, pi_2 - get<1>(p));
         return res;
     }
 };
 
-
-// TODO: specialize collected_vector for geographic_tag
+// TODO: implement collected_vector type for geographic
 
 
 #ifndef DOXYGEN_NO_DETAIL
diff --git a/include/boost/geometry/algorithms/detail/equals/implementation.hpp b/include/boost/geometry/algorithms/detail/equals/implementation.hpp
index 024d6be40..d3b95753f 100644
--- a/include/boost/geometry/algorithms/detail/equals/implementation.hpp
+++ b/include/boost/geometry/algorithms/detail/equals/implementation.hpp
@@ -167,6 +167,12 @@ struct equals_by_collection
                              Geometry2 const& geometry2,
                              Strategy const& strategy)
     {
+        using cs_tag = typename Strategy::cs_tag;
+
+        static_assert(std::is_same<cs_tag, spherical_tag>::value
+                      || std::is_same<cs_tag, cartesian_tag>::value,
+                      "requires a strategy for cartesian or spherical");
+
         if (! TrivialCheck::apply(geometry1, geometry2, strategy))
         {
             return false;
@@ -181,9 +187,15 @@ struct equals_by_collection
                 double
             >::type;
 
-        using collected_vector_type = geometry::collected_vector
+        using collected_vector_type = std::conditional_t
             <
-                calculation_type, Geometry1
+                std::is_same<cs_tag, spherical_tag>::value,
+                collected_vector_spherical
+                    <
+                        calculation_type,
+                        typename geometry::point_type<Geometry1>::type
+                    >,
+                collected_vector_cartesian<calculation_type>
             >;
 
         std::vector<collected_vector_type> c1, c2;
@@ -199,7 +211,7 @@ struct equals_by_collection
         std::sort(c1.begin(), c1.end());
         std::sort(c2.begin(), c2.end());
 
-        // Just check if these vectors are equal.
+        // Check if these vectors are equal.
         return std::equal(c1.begin(), c1.end(), c2.begin());
     }
 };
@@ -218,26 +230,19 @@ struct equals_by_relate
 template <typename Strategy, typename CsTag>
 struct use_collect_vectors
 {
-  static constexpr bool value = false;
+    static constexpr bool value = false;
 };
 
 template <typename Strategy>
 struct use_collect_vectors<Strategy, cartesian_tag>
 {
-  static constexpr bool value = true;
+    static constexpr bool value = true;
 };
 
 template <typename CV, typename CsTag>
 struct use_collect_vectors<strategy::side::spherical_side_formula<CV>, CsTag>
 {
-  static constexpr bool value = true;
-};
-
-template <typename CV>
-struct use_collect_vectors<strategy::side::spherical_side_formula<CV>,
-        geographic_tag>
-{
-  static constexpr bool value = false;
+    static constexpr bool value = true;
 };
 
 // Use either collect_vectors or relate
@@ -255,8 +260,11 @@ struct equals_by_collection_or_relate
         using side_strategy = decltype(std::declval<Strategy>().side());
         using implementation = std::conditional_t
             <
-                use_collect_vectors<side_strategy,
-                     typename cs_tag<Geometry1>::type>::value,
+                use_collect_vectors
+                <
+                    side_strategy,
+                    typename Strategy::cs_tag
+                >::value,
                 equals_by_collection<TrivialCheck>,
                 equals_by_relate<Geometry1, Geometry2>
             >;
diff --git a/test/algorithms/equals/Jamfile b/test/algorithms/equals/Jamfile
index f128597e2..d8585cee8 100644
--- a/test/algorithms/equals/Jamfile
+++ b/test/algorithms/equals/Jamfile
@@ -19,4 +19,5 @@ test-suite boost-geometry-algorithms-equals
     [ run equals.cpp             : : : : algorithms_equals ]
     [ run equals_multi.cpp       : : : : algorithms_equals_multi ]
     [ run equals_on_spheroid.cpp : : : : algorithms_equals_on_spheroid ]
+    [ run equals_sph.cpp         : : : : algorithms_equals_sph ]
     ;
diff --git a/test/algorithms/equals/equals_on_spheroid.cpp b/test/algorithms/equals/equals_on_spheroid.cpp
index 68f9bf462..ce87e0e9b 100644
--- a/test/algorithms/equals/equals_on_spheroid.cpp
+++ b/test/algorithms/equals/equals_on_spheroid.cpp
@@ -234,7 +234,6 @@ BOOST_AUTO_TEST_CASE( equals_segment_segment_geo )
     test_segment_segment<bgm::point<long double, 2, cs_type> >("geo");
 }
 
-#if defined(BOOST_GEOMETRY_TEST_FAILURES)
 // This version uses collect_vectors (because its side
 // strategy is spherical_side_formula) and fails
 BOOST_AUTO_TEST_CASE( equals_ring_ring_se)
@@ -247,7 +246,6 @@ BOOST_AUTO_TEST_CASE( equals_ring_ring_se)
                                         "POLYGON((10 50,10 51,11 50,10 50))",
                                         true);
 }
-#endif
 
 BOOST_AUTO_TEST_CASE( equals_ring_ring_geo )
 {
diff --git a/test/algorithms/equals/equals_sph.cpp b/test/algorithms/equals/equals_sph.cpp
index 45b0227b8..3754535d3 100644
--- a/test/algorithms/equals/equals_sph.cpp
+++ b/test/algorithms/equals/equals_sph.cpp
@@ -147,5 +147,13 @@ int test_main( int , char* [] )
 {
     test_all<bg::model::point<double, 2, bg::cs::spherical_equatorial<bg::degree> > >();
 
+    // TODO: the polar version needs several traits more, for example in cs_tag,
+    // to compile properly.
+    //test_all<bg::model::point<double, 2, bg::cs::polar<bg::degree> > >();
+
+#if defined(BOOST_GEOMETRY_TEST_FAILURES)
+    test_all<bg::model::point<double, 2, bg::cs::geographic<bg::degree> > >();
+#endif
+
     return 0;
 }
diff --git a/test/algorithms/equals/test_equals.hpp b/test/algorithms/equals/test_equals.hpp
index 5eced7f90..f3bdb6a96 100644
--- a/test/algorithms/equals/test_equals.hpp
+++ b/test/algorithms/equals/test_equals.hpp
@@ -26,7 +26,10 @@
 #include <boost/variant/variant.hpp>
 
 
-struct no_strategy {};
+struct no_strategy
+{
+    using cs_tag = void;
+};
 
 template <typename Geometry1, typename Geometry2, typename Strategy>
 bool call_equals(Geometry1 const& geometry1,
@@ -60,7 +63,9 @@ void check_geometry(Geometry1 const& geometry1,
         << " equals: " << wkt1
         << " to " << wkt2
         << " -> Expected: " << expected
-        << " detected: " << detected);
+        << " detected: " << detected
+        << " strategy: " << typeid(Strategy).name()
+        << " cs: " << typeid(typename Strategy::cs_tag).name());
 
     detected = call_equals(geometry2, geometry1, strategy);
 
@@ -69,7 +74,8 @@ void check_geometry(Geometry1 const& geometry1,
         << " equals: " << wkt2
         << " to " << wkt1
         << " -> Expected: " << expected
-        << " detected: " << detected);
+        << " strategy: " << typeid(Strategy).name()
+        << " cs: " << typeid(typename Strategy::cs_tag).name());
 }