Merge pull request #1148 from PowerGridModel/feature/iterator-facade-deducing-this

Modernize internal implementation: iterator facade deducing this

Author: TonyXiang8787

power_grid_model_c/power_grid_model/include/power_grid_model/auxiliary/dataset.hpp

@@ -117,28 +117,38 @@ class ColumnarAttributeRange : public std::ranges::view_interface<ColumnarAttrib
         std::span<AttributeBuffer<Data> const> attribute_buffers_{};
     };
 
-    class iterator
-        : public IteratorFacade<iterator, std::conditional_t<is_data_mutable_v<dataset_type>, Proxy, Proxy const>,
-                                Idx> {
+    class iterator : public IteratorFacade {
       public:
-        using value_type = Proxy;
+        using value_type = std::conditional_t<is_data_mutable_v<dataset_type>, Proxy, Proxy const>;
         using difference_type = Idx;
+        using pointer = std::add_pointer_t<value_type>;
+        using reference = std::add_lvalue_reference_t<value_type>;
 
-        iterator() = default;
+        iterator() : IteratorFacade{*this} {};
         iterator(difference_type idx, std::span<AttributeBuffer<Data> const> attribute_buffers)
-            : current_{idx, attribute_buffers} {}
+            : IteratorFacade{*this}, current_{idx, attribute_buffers} {}
 
-      private:
-        friend class IteratorFacade<iterator, std::conditional_t<is_data_mutable_v<dataset_type>, Proxy, Proxy const>,
-                                    Idx>;
-
-        constexpr auto dereference() -> value_type& { return current_; }
-        constexpr auto dereference() const -> std::add_lvalue_reference_t<std::add_const_t<value_type>> {
+        constexpr auto operator*() -> reference { return current_; }
+        constexpr auto operator*() const -> std::add_lvalue_reference_t<std::add_const_t<value_type>> {
             return current_;
         }
-        constexpr auto three_way_compare(iterator const& other) const { return current_.idx_ <=> other.current_.idx_; }
-        constexpr auto distance_to(iterator const& other) const { return other.current_.idx_ - current_.idx_; }
-        constexpr void advance(difference_type n) { current_.idx_ += n; }
+
+        friend constexpr auto operator<=>(iterator const& first, iterator const& second) {
+            return first.current_idx() <=> second.current_idx();
+        }
+
+        constexpr auto operator+=(difference_type n) -> std::add_lvalue_reference_t<iterator> {
+            current_idx() += n;
+            return *this;
+        }
+
+        friend constexpr auto operator-(iterator const& first, iterator const& second) -> difference_type {
+            return first.current_idx() - second.current_idx();
+        }
+
+      private:
+        constexpr auto current_idx() const { return current_.idx_; }
+        constexpr auto& current_idx() { return current_.idx_; }
 
         Proxy current_;
     };

power_grid_model_c/power_grid_model/include/power_grid_model/common/grouped_index_vector.hpp

@@ -94,26 +94,20 @@ constexpr auto from_dense = from_dense_t{};
 
 class SparseGroupedIdxVector {
   private:
-    class GroupIterator : public IteratorFacade<GroupIterator, IdxRange const, Idx> {
-        friend class IteratorFacade<GroupIterator, IdxRange const, Idx>;
-        using Base = IteratorFacade<GroupIterator, IdxRange const, Idx>;
-
+    class GroupIterator : public IteratorFacade {
       public:
-        using value_type = typename Base::value_type;
-        using difference_type = typename Base::difference_type;
-        using reference = typename Base::reference;
-
-        GroupIterator() = default;
-        explicit constexpr GroupIterator(IdxVector const& indptr, Idx group) : indptr_{&indptr}, group_{group} {}
-
-      private:
-        IdxVector const* indptr_{};
-        Idx group_{};
-        mutable value_type latest_value_{}; // making this mutable allows us to delay out-of-bounds checks until
-                                            // dereferencing instead of update methods. Note that the value will be
-                                            // invalidated at first update
-
-        constexpr auto dereference() const -> reference {
+        using iterator = GroupIterator;
+        using const_iterator = std::add_const_t<GroupIterator>;
+        using value_type = IdxRange const;
+        using difference_type = Idx;
+        using pointer = std::add_pointer_t<value_type>;
+        using reference = std::add_lvalue_reference_t<value_type>;
+
+        GroupIterator() : IteratorFacade{*this} {};
+        explicit constexpr GroupIterator(IdxVector const& indptr, Idx group)
+            : IteratorFacade{*this}, indptr_{&indptr}, group_{group} {}
+
+        constexpr auto operator*() const -> reference {
             assert(indptr_ != nullptr);
             assert(0 <= group_);
             assert(group_ < static_cast<Idx>(indptr_->size() - 1));
@@ -123,15 +117,29 @@ class SparseGroupedIdxVector {
             latest_value_ = value_type{(*indptr_)[group_], (*indptr_)[group_ + 1]};
             return latest_value_;
         }
-        constexpr std::strong_ordering three_way_compare(GroupIterator const& other) const {
-            assert(indptr_ == other.indptr_);
-            return group_ <=> other.group_;
+
+        friend constexpr std::strong_ordering operator<=>(GroupIterator const& first, GroupIterator const& second) {
+            assert(first.indptr_ == second.indptr_);
+            return first.group_ <=> second.group_;
+        }
+
+        constexpr auto operator+=(difference_type n) -> std::add_lvalue_reference_t<GroupIterator> {
+            group_ += n;
+            return *this;
         }
-        constexpr auto distance_to(GroupIterator const& other) const {
-            assert(indptr_ == other.indptr_);
-            return other.group_ - group_;
+
+        friend auto operator-(GroupIterator const& first, GroupIterator const& second) -> difference_type {
+            assert(first.indptr_ == second.indptr_);
+            return first.group_ - second.group_;
         }
-        constexpr void advance(Idx n) { group_ += n; }
+
+      private:
+        IdxVector const* indptr_{};
+        Idx group_{};
+        mutable std::remove_const_t<value_type>
+            latest_value_{}; // making this mutable allows us to delay out-of-bounds checks until
+                             // dereferencing instead of update methods. Note that the value will be
+                             // invalidated at first update
     };
 
     constexpr auto group_iterator(Idx group) const { return GroupIterator{indptr_, group}; }
@@ -168,32 +176,25 @@ class SparseGroupedIdxVector {
 
 class DenseGroupedIdxVector {
   private:
-    class GroupIterator : public IteratorFacade<GroupIterator, IdxRange const, Idx> {
-        friend class IteratorFacade<GroupIterator, IdxRange const, Idx>;
-        using Base = IteratorFacade<GroupIterator, IdxRange const, Idx>;
+    class GroupIterator : public IteratorFacade {
+        friend class IteratorFacade; // to expose increment and decrement
 
       public:
-        using value_type = typename Base::value_type;
-        using difference_type = typename Base::difference_type;
-        using reference = typename Base::reference;
-
-        GroupIterator() = default;
+        using iterator = GroupIterator;
+        using const_iterator = std::add_const_t<GroupIterator>;
+        using value_type = IdxRange const;
+        using difference_type = Idx;
+        using pointer = std::add_pointer_t<value_type>;
+        using reference = std::add_lvalue_reference_t<value_type>;
+
+        GroupIterator() : IteratorFacade{*this} {};
         explicit constexpr GroupIterator(IdxVector const& dense_vector, Idx group)
-            : dense_vector_{&dense_vector},
+            : IteratorFacade{*this},
+              dense_vector_{&dense_vector},
               group_{group},
               group_range_{std::ranges::equal_range(*dense_vector_, group)} {}
 
-      private:
-        using group_iterator = IdxVector::const_iterator;
-
-        IdxVector const* dense_vector_{};
-        Idx group_{};
-        std::ranges::subrange<group_iterator> group_range_;
-        mutable value_type latest_value_{}; // making this mutable allows us to delay out-of-bounds checks until
-                                            // dereferencing instead of update methods. Note that the value will be
-                                            // invalidated at first update
-
-        constexpr auto dereference() const -> reference {
+        constexpr auto operator*() const -> reference {
             assert(dense_vector_ != nullptr);
 
             // delaying out-of-bounds checking until dereferencing while still returning a reference type requires
@@ -203,10 +204,31 @@ class DenseGroupedIdxVector {
                            narrow_cast<Idx>(std::distance(std::cbegin(*dense_vector_), group_range_.end()))};
             return latest_value_;
         }
-        constexpr std::strong_ordering three_way_compare(GroupIterator const& other) const {
-            return group_ <=> other.group_;
+
+        friend constexpr std::strong_ordering operator<=>(GroupIterator const& first, GroupIterator const& second) {
+            assert(first.dense_vector_ == second.dense_vector_);
+            return first.group_ <=> second.group_;
+        }
+
+        constexpr auto operator+=(difference_type n) -> std::add_lvalue_reference_t<GroupIterator> {
+            advance(n);
+            return *this;
         }
-        constexpr auto distance_to(GroupIterator const& other) const { return other.group_ - group_; }
+        friend auto operator-(GroupIterator const& first, GroupIterator const& second) -> difference_type {
+            assert(first.dense_vector_ == second.dense_vector_);
+            return first.group_ - second.group_;
+        }
+
+      private:
+        using group_iterator = IdxVector::const_iterator;
+
+        IdxVector const* dense_vector_{};
+        Idx group_{};
+        std::ranges::subrange<group_iterator> group_range_;
+        mutable std::remove_const_t<value_type>
+            latest_value_{}; // making this mutable allows us to delay out-of-bounds checks until
+                             // dereferencing instead of update methods. Note that the value will be
+                             // invalidated at first update
 
         constexpr void increment() {
             ++group_;
@@ -220,7 +242,7 @@ class DenseGroupedIdxVector {
                                 .base(),
                             group_range_.begin()};
         }
-        constexpr void advance(Idx n) {
+        constexpr void advance(difference_type n) {
             auto const start = n > 0 ? group_range_.end() : std::cbegin(*dense_vector_);
             auto const stop = n < 0 ? group_range_.begin() : std::cend(*dense_vector_);
 

power_grid_model_c/power_grid_model/include/power_grid_model/common/iterator_facade.hpp

@@ -8,93 +8,113 @@
 
 #include <concepts>
 #include <ranges>
+#include <utility>
 
 namespace power_grid_model {
-template <class Impl, typename ValueType, std::integral DifferenceType> class IteratorFacade {
+namespace detail {
+template <typename T>
+concept iterator_facadeable_c = std::integral<typename T::difference_type> && std::is_pointer_v<typename T::pointer> &&
+                                std::is_lvalue_reference_v<typename T::reference> &&
+                                requires(T t, std::remove_cvref_t<T> mt, std::add_const_t<T> ct,
+                                         std::add_const_t<T> ct2, typename std::remove_cvref_t<T>::difference_type d) {
+                                    typename T::value_type;
+                                    { *t } -> std::same_as<typename T::reference>;
+                                    { &*t } -> std::same_as<typename T::pointer>;
+                                    { ct <=> ct2 } -> std::same_as<std::strong_ordering>;
+                                    { mt += d } -> std::same_as<std::add_lvalue_reference_t<T>>;
+                                    { ct - ct2 } -> std::same_as<typename T::difference_type>;
+                                };
+
+template <typename T, typename Int>
+concept compound_assignable_c = requires(T t, Int n) {
+    { t += n } -> std::same_as<std::add_lvalue_reference_t<T>>;
+};
+} // namespace detail
+
+class IteratorFacade {
   public:
-    using iterator = Impl; // CRTP
-    using const_iterator = std::add_const_t<iterator>;
-    using value_type = std::remove_cvref_t<ValueType>;
-    using difference_type = DifferenceType;
     using iterator_category = std::random_access_iterator_tag;
-    using pointer = std::add_pointer_t<ValueType>;
-    using reference = std::add_lvalue_reference_t<ValueType>;
 
-    constexpr auto operator*() const -> decltype(auto) { return static_cast<const_iterator*>(this)->dereference(); }
-    constexpr auto operator*() -> reference
-        requires requires(iterator it) {
-            { it.dereference() } -> std::same_as<reference>;
-        }
-    {
-        return static_cast<iterator*>(this)->dereference();
+    template <typename Self> constexpr decltype(auto) operator->(this Self&& self) {
+        return &(*std::forward<Self>(self));
     }
-    constexpr auto operator->() const -> decltype(auto) { return &(*(*this)); }
-    constexpr auto operator->() -> decltype(auto) { return &(*(*this)); }
 
-    friend constexpr bool operator==(IteratorFacade const& first, IteratorFacade const& second) {
-        return (first <=> second) == std::strong_ordering::equivalent;
-    }
-    friend constexpr std::strong_ordering operator<=>(IteratorFacade const& first, IteratorFacade const& second) {
-        return first.three_way_compare(second);
+    template <typename Self, typename Other>
+        requires std::same_as<std::remove_cvref_t<Self>, std::remove_cvref_t<Other>>
+    constexpr bool operator==(this Self const& self, Other const& other) {
+        return (self <=> other) == std::strong_ordering::equivalent;
     }
 
-    constexpr auto operator++() -> iterator& {
-        if constexpr (requires(iterator it) { it.increment(); }) {
-            static_cast<iterator*>(this)->increment();
+    // NOLINTNEXTLINE(cert-dcl21-cpp) // pre-decrement but clang-tidy incorrectly sees this as post-decrement
+    template <typename Self> constexpr std::add_lvalue_reference_t<Self> operator++(this Self& self) {
+        if constexpr (requires { self.increment(); }) { // NOTE: IteratorFacade should be a friend class
+            self.increment();
         } else {
-            static_cast<iterator*>(this)->advance(1);
+            return (self += 1);
         }
-        return *static_cast<iterator*>(this);
+        return self;
     }
-    constexpr auto operator--() -> iterator& {
-        if constexpr (requires(iterator it) { it.decrement(); }) {
-            static_cast<iterator*>(this)->decrement();
+    // NOLINTNEXTLINE(cert-dcl21-cpp) // pre-decrement but clang-tidy incorrectly sees this as post-decrement
+    template <typename Self> constexpr std::add_lvalue_reference_t<Self> operator--(this Self& self) {
+        if constexpr (requires { self.decrement(); }) { // NOTE: IteratorFacade should be a friend class
+            self.decrement();
         } else {
-            static_cast<iterator*>(this)->advance(-1);
+            return (self += -1);
         }
-        return *static_cast<iterator*>(this);
+        return self;
     }
-    constexpr auto operator++(std::integral auto /*idx*/) -> iterator {
-        iterator result{*static_cast<iterator*>(this)};
-        ++(*this);
+    template <typename Self>
+    constexpr std::remove_cvref_t<Self> operator++(this Self& self, std::integral auto /*idx*/) {
+        using Result = std::remove_cvref_t<Self>;
+        Result result{self};
+        ++self;
         return result;
     }
-    constexpr auto operator--(std::integral auto /*idx*/) -> iterator {
-        iterator result{*static_cast<iterator*>(this)};
-        --(*this);
+    template <typename Self>
+    constexpr std::remove_cvref_t<Self> operator--(this Self& self, std::integral auto /*idx*/) {
+        using Result = std::remove_cvref_t<Self>;
+        Result result{self};
+        --self;
         return result;
     }
-    constexpr auto operator+=(std::integral auto offset) -> iterator& {
-        static_cast<iterator*>(this)->advance(offset);
-        return *static_cast<iterator*>(this);
+    template <typename Self>
+    constexpr std::add_lvalue_reference_t<Self> operator-=(this Self& self, std::integral auto idx) {
+        return (self += (-idx));
     }
-    constexpr auto operator-=(std::integral auto idx) -> iterator& { return ((*this) += (-idx)); }
 
-    friend constexpr auto operator+(iterator const& it, difference_type offset) -> iterator {
-        iterator result{it};
+    template <typename Self, std::integral Int>
+    friend constexpr std::remove_cvref_t<Self> operator+(Self&& self, Int offset)
+        requires std::derived_from<std::remove_cvref_t<Self>, IteratorFacade> //&& detail::iterator_facadeable_c<Self>
+    {
+        using Result = std::remove_cvref_t<Self>;
+        Result result{std::forward<Self>(self)};
         result += offset;
         return result;
     }
-    friend constexpr auto operator+(difference_type offset, iterator it) -> iterator { return (it += offset); }
-    friend constexpr auto operator-(iterator const& it, difference_type idx) -> iterator { return it + (-idx); }
-    friend constexpr auto operator-(IteratorFacade const& first, IteratorFacade const& second) -> difference_type {
-        return second.distance_to(first);
+    template <typename Self, std::integral Int>
+        requires std::derived_from<std::remove_cvref_t<Self>, IteratorFacade> //&& detail::iterator_facadeable_c<Self>
+    friend constexpr std::remove_cvref_t<Self> operator+(Int offset, Self&& self) {
+        return std::forward<Self>(self) + offset;
+    }
+    template <typename Self, std::integral Int>
+        requires std::derived_from<std::remove_cvref_t<Self>, IteratorFacade> //&& detail::iterator_facadeable_c<Self>
+    friend constexpr std::remove_cvref_t<Self> operator-(Self&& self, Int idx) {
+        return (std::forward<Self>(self)) + (-idx);
     }
 
-    constexpr auto operator[](difference_type idx) const -> value_type const& { return *(*this + idx); }
-
-  private:
-    IteratorFacade() = default; // default constructor is private to prevent non-CRTP instantiation
-    friend Impl;                // allow Impl to access private members; this is necessary for CRTP
-
-    // overloads for public bidirectional exposure (difference between MSVC and ClangCL)
-    constexpr std::strong_ordering three_way_compare(IteratorFacade const& other) const {
-        return static_cast<std::add_lvalue_reference_t<const_iterator>>(*this).three_way_compare(
-            static_cast<std::add_lvalue_reference_t<const_iterator>>(other));
+    template <typename Self>
+    constexpr decltype(auto) operator[](this Self const& self, typename Self::difference_type idx) {
+        return *(self + idx);
     }
-    constexpr auto distance_to(IteratorFacade const& other) const -> difference_type {
-        return static_cast<std::add_lvalue_reference_t<const_iterator>>(*this).distance_to(
-            static_cast<std::add_lvalue_reference_t<const_iterator>>(other));
+
+    // prevent construction by non-derived and non-iterator-facadeable types
+    IteratorFacade() = delete;
+    template <typename Self> constexpr explicit IteratorFacade(Self& /*self*/) {
+        // cannot be done using constraints because the type is not fully instantiated yet when the compiler
+        // instantiates the constructor. Note that this is different from the other methods because those are only
+        // instantiated when used.
+        static_assert(std::derived_from<std::remove_cvref_t<Self>, IteratorFacade>);
+        static_assert(detail::iterator_facadeable_c<std::remove_cvref_t<Self>>);
     }
 };