From f1487b01da6b0abccf9eb9845d5b62497dc8b405 Mon Sep 17 00:00:00 2001 From: Ben Jude Date: Mon, 7 Nov 2022 22:50:52 +1100 Subject: [PATCH 1/2] Modify RangeIteratorBase to (nearly) conform to std forward iterator requirements This allows the various set ranges to be used with std algorithms. These iterators differ from the LegacyForwardIterator requirements (https://en.cppreference.com/w/cpp/named_req/ForwardIterator) in a couple of ways - No operator->: Since these iterators construct their elements on dereference, theres not safe way to return pointer to one outside of using a proxy object. Operator-> isnt required for c++20's std::input_iterator concept so shouldnt be an issue for std algorithms - std::iterator_traits>::reference is not a reference: Similar reasoning to above, it could be defined as `const value_type&` but that could be asking for a dangling reference. This is also not a requirement for c++20's std::forward_iterator --- .../utilities/element_iterators.h | 12 ++- .../utilities/element_iterators.ipp | 11 +- test/CMakeLists.txt | 1 + test/src/halfedge_iterator_categories.cpp | 102 ++++++++++++++++++ 4 files changed, 122 insertions(+), 4 deletions(-) create mode 100644 test/src/halfedge_iterator_categories.cpp diff --git a/include/geometrycentral/utilities/element_iterators.h b/include/geometrycentral/utilities/element_iterators.h index 4cbb59d9..90fb9ef8 100644 --- a/include/geometrycentral/utilities/element_iterators.h +++ b/include/geometrycentral/utilities/element_iterators.h @@ -31,11 +31,19 @@ template class RangeIteratorBase { public: + using difference_type = std::ptrdiff_t; + using value_type = typename F::Etype; + using pointer = value_type*; + using reference = value_type; + using iterator_category = std::forward_iterator_tag; + + RangeIteratorBase() = default; RangeIteratorBase(typename F::ParentMeshT* mesh_, size_t iStart_, size_t iEnd_); - const RangeIteratorBase& operator++(); + RangeIteratorBase& operator++(); + RangeIteratorBase operator++(int); bool operator==(const RangeIteratorBase& other) const; bool operator!=(const RangeIteratorBase& other) const; - typename F::Etype operator*() const; + reference operator*() const; private: typename F::ParentMeshT* mesh; diff --git a/include/geometrycentral/utilities/element_iterators.ipp b/include/geometrycentral/utilities/element_iterators.ipp index 19959051..469c337f 100644 --- a/include/geometrycentral/utilities/element_iterators.ipp +++ b/include/geometrycentral/utilities/element_iterators.ipp @@ -13,7 +13,7 @@ inline RangeIteratorBase::RangeIteratorBase(typename F::ParentMeshT* mesh_, s } template -inline const RangeIteratorBase& RangeIteratorBase::operator++() { +inline RangeIteratorBase& RangeIteratorBase::operator++() { iCurr++; while (iCurr != iEnd && !F::elementOkay(*mesh, iCurr)) { iCurr++; @@ -21,6 +21,13 @@ inline const RangeIteratorBase& RangeIteratorBase::operator++() { return *this; } +template +inline RangeIteratorBase RangeIteratorBase::operator++(int) { + auto ret = *this; + ++(*this); + return ret; +} + template inline bool RangeIteratorBase::operator==(const RangeIteratorBase& other) const { return iCurr == other.iCurr; @@ -32,7 +39,7 @@ inline bool RangeIteratorBase::operator!=(const RangeIteratorBase& other) } template -inline typename F::Etype RangeIteratorBase::operator*() const { +inline typename RangeIteratorBase::reference RangeIteratorBase::operator*() const { return typename F::Etype(mesh, iCurr); } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index f8ef6d7e..dbc0db35 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -93,6 +93,7 @@ set(TEST_SRCS src/main_test.cpp src/load_test_meshes.cpp src/eigen_interop_helpers_test.cpp + src/halfedge_iterator_categories.cpp src/halfedge_mesh_test.cpp src/halfedge_mutation_test.cpp src/halfedge_geometry_test.cpp diff --git a/test/src/halfedge_iterator_categories.cpp b/test/src/halfedge_iterator_categories.cpp new file mode 100644 index 00000000..632510e1 --- /dev/null +++ b/test/src/halfedge_iterator_categories.cpp @@ -0,0 +1,102 @@ +#include "geometrycentral/surface/halfedge_element_types.h" + +#include "gtest/gtest.h" + +#include + +using namespace geometrycentral; +using namespace geometrycentral::surface; + +// ============================================================ +// =============== Helper Type Traits +// ============================================================ + +// std::void_t is not in the standard library until c++17 +template +using void_t = void; + +// std::is_swappable is not in the standard library until c++17 +template +struct is_swappable : std::false_type {}; +template +struct is_swappable(), std::declval()))>> : std::true_type {}; + +template +struct is_dereferencable : std::false_type {}; +template +struct is_dereferencable())>> : std::true_type {}; + +template +struct is_pre_incrementable : std::false_type {}; +template +struct is_pre_incrementable())>> : std::true_type {}; + +template +struct is_post_incrementable : std::false_type {}; +template +struct is_post_incrementable()++)>> : std::true_type {}; + +// ============================================================ +// =============== Templated Test Setup +// ============================================================ + +template +class StlIteratorCategories : public ::testing::Test {}; + +using GCRangeTypes = ::testing::Types; +TYPED_TEST_SUITE(StlIteratorCategories, GCRangeTypes); + + +// ============================================================ +// =============== Templated Test Cases +// ============================================================ + +TYPED_TEST(StlIteratorCategories, InputIteratorConcept) { + using iterator_t = decltype(std::declval().begin()); + + // LegacyIterator requirements + EXPECT_TRUE(std::is_copy_constructible::value); + EXPECT_TRUE(std::is_copy_assignable::value); + EXPECT_TRUE(std::is_destructible::value); + EXPECT_TRUE(is_swappable::value); + // Check for iterator_traits typedefs + using value_type = typename std::iterator_traits::value_type; + using difference_type = typename std::iterator_traits::difference_type; + using pointer = typename std::iterator_traits::pointer; + using reference = typename std::iterator_traits::reference; + using iterator_category = typename std::iterator_traits::iterator_category; + EXPECT_TRUE(is_dereferencable::value); + EXPECT_TRUE(is_pre_incrementable::value); + + // LegacyInputIterator + // - operator-> is not checked for, the RangeIteratorBase and NavigationIteratorBase create their values on + // dereference, so returning a pointer to them is asking for a dangling pointer. operator-> is not required to meet + // the c++20 std::input_iterator concept so in practice not having this shouldnt affect the ability to use std + // algorithms. + EXPECT_TRUE((std::is_convertible() != std::declval()), bool>::value)); + EXPECT_TRUE((std::is_convertible()), value_type>::value)); + EXPECT_TRUE(is_post_incrementable::value); + EXPECT_TRUE((std::is_convertible()++), value_type>::value)); + EXPECT_TRUE((std::is_base_of::value)); +} + +TYPED_TEST(StlIteratorCategories, ForwardIteratorConcept) { + using iterator_t = decltype(std::declval().begin()); + + using value_type = std::iterator_traits::value_type; + using reference = std::iterator_traits::reference; + using iterator_category = std::iterator_traits::iterator_category; + // LegacyForwardIterator requirements + // Note: There are a number of LegacyForwardIterator requirements that we do not enforce here: + // - RangeIteratorBase::reference is not a reference type as the values are generated on dereference, returning a + // reference does not make sense + // - Because of the above, the multipass guarantee is not strictly met. The value returned from dereferencing two + // iterators that compare equal will compare equal, but they will not be references to the same object. In practice + // this doesnt matter as far as using std algorithms + EXPECT_TRUE(std::is_default_constructible::value); + EXPECT_TRUE((std::is_same()++), iterator_t>::value)); + EXPECT_TRUE((std::is_same()++), reference>::value)); + EXPECT_TRUE((std::is_same::type, value_type>::value)); + EXPECT_TRUE((std::is_base_of::value)); +} \ No newline at end of file From bae9426d70e0f7250e8a602823c15d5e9c1761e9 Mon Sep 17 00:00:00 2001 From: Ben Jude Date: Mon, 7 Nov 2022 22:54:46 +1100 Subject: [PATCH 2/2] Modify NavigationIteratorBase to (almost) conform to std forward iterator requirements The `const bool` in `VertexNeighborIteratorState` was causing the default constructor for `NavigationIteratorBase` and friends by implicitly deleting default constructors up the chain. This was solved by making the member variables private and adding `const` accessors. The functionality of the class should be the same, just a few renamings to avoid clashes --- .../surface/halfedge_element_types.h | 19 +++++++++++----- .../surface/halfedge_element_types.ipp | 22 +++++++++---------- .../utilities/element_iterators.h | 15 ++++++++++--- .../utilities/element_iterators.ipp | 11 ++++++++-- test/src/halfedge_iterator_categories.cpp | 13 +++++++++-- 5 files changed, 56 insertions(+), 24 deletions(-) diff --git a/include/geometrycentral/surface/halfedge_element_types.h b/include/geometrycentral/surface/halfedge_element_types.h index 70fc8542..75b9831b 100644 --- a/include/geometrycentral/surface/halfedge_element_types.h +++ b/include/geometrycentral/surface/halfedge_element_types.h @@ -333,18 +333,25 @@ typedef RangeSetBase BoundaryLoopSet; // For implicit-twin meshes, it just does the usual twin.next() and currHe is always an outgoing halfedge. For general // meshes, it iterates first through the outgoing, then the incoming halfedges (always stored in currHe). struct VertexNeighborIteratorState { + VertexNeighborIteratorState() = default; VertexNeighborIteratorState(Halfedge currHeOutgoing, bool useImplicitTwin); - const bool useImplicitTwin; + void advance(); + bool isHalfedgeCanonical() const; // this currently pointing at the one canonical halfedge along an edge + bool operator==(const VertexNeighborIteratorState& rhs) const; + + bool useImplicitTwin() const { return useImplicitTwinFlag; } + bool processingIncoming() const { return processingIncomingFlag; } + Halfedge firstHalfedge() const { return firstHe; } + Halfedge currentHalfedge() const { return currHe; } + +private: + bool useImplicitTwinFlag; Halfedge currHe = Halfedge(); // if useImplicitTwin == false, this is populated - bool processingIncoming = false; + bool processingIncomingFlag = false; Halfedge firstHe = Halfedge(); - - void advance(); - bool isHalfedgeCanonical() const; // this currently pointing at the one canonical halfedge along an edge - bool operator==(const VertexNeighborIteratorState& rhs) const; }; // Adjacent vertices diff --git a/include/geometrycentral/surface/halfedge_element_types.ipp b/include/geometrycentral/surface/halfedge_element_types.ipp index cea15c1e..362262de 100644 --- a/include/geometrycentral/surface/halfedge_element_types.ipp +++ b/include/geometrycentral/surface/halfedge_element_types.ipp @@ -134,18 +134,18 @@ inline bool VertexRangeF::elementOkay(const SurfaceMesh& mesh, size_t ind) { // Our data structure offers iteration around the outgoing halfedges from each vertex, which takes care of outoing halfedges. Furthermore one thing we know is that for every face in which the vertex appears, there is at least one incoming and one outgoing halfedge. We can use this to find the faces and outgoing halfedges. For edges and vertices, // In both the mannifold and nonmanifold case, if a vertex appears in a face multiple times, (aka its a Delta-complex), then these iterators will return elements multiple times. -inline VertexNeighborIteratorState::VertexNeighborIteratorState(Halfedge currHe_, bool useImplicitTwin_) : useImplicitTwin(useImplicitTwin_), currHe(currHe_), firstHe(currHe_) {} +inline VertexNeighborIteratorState::VertexNeighborIteratorState(Halfedge currHe_, bool useImplicitTwin_) : useImplicitTwinFlag(useImplicitTwin_), currHe(currHe_), firstHe(currHe_) {} // clang-format on inline void VertexNeighborIteratorState::advance() { - if (useImplicitTwin) { + if (useImplicitTwinFlag) { currHe = currHe.nextOutgoingNeighbor(); // twin().next() } else { - if (!processingIncoming) { + if (!processingIncomingFlag) { // this happens first currHe = currHe.nextOutgoingNeighbor(); if (currHe == firstHe) { // switch to processing incoming if needed - processingIncoming = true; + processingIncomingFlag = true; currHe = firstHe.prevOrbitFace(); firstHe = currHe; } @@ -153,7 +153,7 @@ inline void VertexNeighborIteratorState::advance() { // this happens second currHe = currHe.nextIncomingNeighbor(); if (currHe == firstHe) { // switch back to processing outgoing if needed (returning to initial state) - processingIncoming = false; + processingIncomingFlag = false; currHe = firstHe.next(); firstHe = currHe; } @@ -164,7 +164,7 @@ inline void VertexNeighborIteratorState::advance() { inline bool VertexNeighborIteratorState::isHalfedgeCanonical() const { // TODO I _think_ that this leads to different Delta-complex behavior on implicit twin vs. without wrt yielding // elements multiple times when there is a self-edge... - if (useImplicitTwin) { + if (useImplicitTwinFlag) { return true; } else { return currHe == currHe.edge().halfedge(); @@ -172,17 +172,17 @@ inline bool VertexNeighborIteratorState::isHalfedgeCanonical() const { } inline bool VertexNeighborIteratorState::operator==(const VertexNeighborIteratorState& rhs) const { - return currHe == rhs.currHe && processingIncoming == rhs.processingIncoming; + return currHe == rhs.currHe && processingIncomingFlag == rhs.processingIncomingFlag; } // clang-format off inline void VertexAdjacentVertexNavigator::advance() { currE.advance(); } inline bool VertexAdjacentVertexNavigator::isValid() const { return currE.isHalfedgeCanonical(); } inline Vertex VertexAdjacentVertexNavigator::getCurrent() const { - if(currE.useImplicitTwin || !currE.processingIncoming) { - return currE.currHe.next().vertex(); + if(currE.useImplicitTwin() || !currE.processingIncoming()) { + return currE.currentHalfedge().next().vertex(); } else { - return currE.currHe.vertex(); + return currE.currentHalfedge().vertex(); } } @@ -200,7 +200,7 @@ inline Corner VertexAdjacentCornerNavigator::getCurrent() const { return currE.c inline void VertexAdjacentEdgeNavigator::advance() { currE.advance(); } inline bool VertexAdjacentEdgeNavigator::isValid() const { return currE.isHalfedgeCanonical(); } -inline Edge VertexAdjacentEdgeNavigator::getCurrent() const { return currE.currHe.edge(); } +inline Edge VertexAdjacentEdgeNavigator::getCurrent() const { return currE.currentHalfedge().edge(); } inline void VertexAdjacentFaceNavigator::advance() { currE = currE.nextOutgoingNeighbor(); } inline bool VertexAdjacentFaceNavigator::isValid() const { return currE.isInterior(); } diff --git a/include/geometrycentral/utilities/element_iterators.h b/include/geometrycentral/utilities/element_iterators.h index 90fb9ef8..2b54adec 100644 --- a/include/geometrycentral/utilities/element_iterators.h +++ b/include/geometrycentral/utilities/element_iterators.h @@ -34,7 +34,7 @@ class RangeIteratorBase { using difference_type = std::ptrdiff_t; using value_type = typename F::Etype; using pointer = value_type*; - using reference = value_type; + using reference = const value_type&; using iterator_category = std::forward_iterator_tag; RangeIteratorBase() = default; @@ -89,11 +89,20 @@ template class NavigationIteratorBase { public: + using difference_type = std::ptrdiff_t; + using value_type = typename N::Rtype; + using pointer = value_type*; + using reference = value_type; + using iterator_category = std::forward_iterator_tag; + + NavigationIteratorBase() = default; + NavigationIteratorBase(typename N::Etype firstE_, bool justStarted_); - const NavigationIteratorBase& operator++(); + NavigationIteratorBase& operator++(); + NavigationIteratorBase operator++(int); bool operator==(const NavigationIteratorBase& other) const; bool operator!=(const NavigationIteratorBase& other) const; - typename N::Rtype operator*() const; + reference operator*() const; private: N state; diff --git a/include/geometrycentral/utilities/element_iterators.ipp b/include/geometrycentral/utilities/element_iterators.ipp index 469c337f..39feb69c 100644 --- a/include/geometrycentral/utilities/element_iterators.ipp +++ b/include/geometrycentral/utilities/element_iterators.ipp @@ -82,7 +82,7 @@ inline NavigationIteratorBase::NavigationIteratorBase(typename N::Etype e, bo } template -inline const NavigationIteratorBase& NavigationIteratorBase::operator++() { +inline NavigationIteratorBase& NavigationIteratorBase::operator++() { state.advance(); while (!state.isValid()) { state.advance(); @@ -91,6 +91,13 @@ inline const NavigationIteratorBase& NavigationIteratorBase::operator++() return *this; } +template +inline NavigationIteratorBase NavigationIteratorBase::operator++(int) { + auto ret = *this; + ++(*this); + return ret; +} + template inline bool NavigationIteratorBase::operator==(const NavigationIteratorBase& other) const { return justStarted == other.justStarted && state.currE == other.state.currE; @@ -102,7 +109,7 @@ inline bool NavigationIteratorBase::operator!=(const NavigationIteratorBase -inline typename N::Rtype NavigationIteratorBase::operator*() const { +inline typename NavigationIteratorBase::reference NavigationIteratorBase::operator*() const { return state.getCurrent(); } diff --git a/test/src/halfedge_iterator_categories.cpp b/test/src/halfedge_iterator_categories.cpp index 632510e1..83fa2a73 100644 --- a/test/src/halfedge_iterator_categories.cpp +++ b/test/src/halfedge_iterator_categories.cpp @@ -43,8 +43,17 @@ struct is_post_incrementable()++)>> : std::t template class StlIteratorCategories : public ::testing::Test {}; -using GCRangeTypes = ::testing::Types; +using GCRangeTypes = ::testing::Types< + HalfedgeSet, HalfedgeInteriorSet, HalfedgeExteriorSet, CornerSet, VertexSet, EdgeSet, FaceSet, BoundaryLoopSet, + NavigationSetBase, NavigationSetBase, + NavigationSetBase, NavigationSetBase, + NavigationSetBase, NavigationSetBase, + NavigationSetBase, NavigationSetBase, + NavigationSetBase, NavigationSetBase, + NavigationSetBase, NavigationSetBase, + NavigationSetBase, NavigationSetBase, + NavigationSetBase, NavigationSetBase, + NavigationSetBase>; TYPED_TEST_SUITE(StlIteratorCategories, GCRangeTypes);