From 568db64d93dbc0e990f69ace7dd087222b381bdf Mon Sep 17 00:00:00 2001 From: Thomas Weidner Date: Fri, 24 Feb 2023 19:42:47 +0100 Subject: [PATCH 1/2] Make operator* and operator-> const in all iterators. C++ iterators basically model pointers. Hence a const pointer can still be dereferenced to a mutable object (`T * const` *not* `const T*`). This is not true for many iterators in itertools since they only have non-`const` versions of `operator*`. This also violates C++ iterator concepts, see [the github issue](#91). This change basically replaces all non-`const` dereference operators with `const` ones. This was straight forward in most cases: - Some iterators own the data they return (note: that probably violates [`LegacyForwardIterator`](https://en.cppreference.com/w/cpp/named_req/ForwardIterator)). So those data fields were changed to `mutable`. - `GroupBy` advances the group while dereferencing. This does not work when the iterator is constant. Moved the advancing to the constructor and increment operators. This is the only real behavior change, please review carefully. --- batched.hpp | 4 ++-- chain.hpp | 26 +++++++++++++------------- chunked.hpp | 4 ++-- combinations.hpp | 6 +++--- combinations_with_replacement.hpp | 6 +++--- compress.hpp | 4 ++-- cycle.hpp | 4 ++-- dropwhile.hpp | 4 ++-- enumerate.hpp | 4 ++-- filter.hpp | 4 ++-- groupby.hpp | 21 ++++++++++++--------- internal/iterator_wrapper.hpp | 1 + permutations.hpp | 6 +++--- powerset.hpp | 4 ++-- product.hpp | 4 ++-- reversed.hpp | 4 ++-- slice.hpp | 4 ++-- sliding_window.hpp | 4 ++-- starmap.hpp | 8 ++++---- takewhile.hpp | 4 ++-- test/helpers.hpp | 6 +++--- zip.hpp | 4 ++-- zip_longest.hpp | 4 ++-- 23 files changed, 72 insertions(+), 68 deletions(-) diff --git a/batched.hpp b/batched.hpp index e5eadf13..7c4e77c2 100644 --- a/batched.hpp +++ b/batched.hpp @@ -132,11 +132,11 @@ class iter::impl::Batcher { && (done() || !(sub_iter_ != other.sub_iter_)); } - DerefVec& operator*() { + DerefVec& operator*() const { return *batch_; } - DerefVec* operator->() { + DerefVec* operator->() const { return batch_.get(); } }; diff --git a/chain.hpp b/chain.hpp index 23d1e054..e26d8044 100644 --- a/chain.hpp +++ b/chain.hpp @@ -1,10 +1,6 @@ #ifndef ITER_CHAIN_HPP_ #define ITER_CHAIN_HPP_ -#include "internal/iter_tuples.hpp" -#include "internal/iterator_wrapper.hpp" -#include "internal/iterbase.hpp" - #include #include #include @@ -12,6 +8,10 @@ #include #include +#include "internal/iter_tuples.hpp" +#include "internal/iterator_wrapper.hpp" +#include "internal/iterbase.hpp" + namespace iter { namespace impl { template @@ -62,12 +62,12 @@ class iter::impl::Chained { using ArrowType = iterator_arrow>; template - static DerefType get_and_deref(IterTupType& iters) { + static DerefType get_and_deref(const IterTupType& iters) { return *std::get(iters); } template - static ArrowType get_and_arrow(IterTupType& iters) { + static ArrowType get_and_arrow(const IterTupType& iters) { return apply_arrow(std::get(iters)); } @@ -82,8 +82,8 @@ class iter::impl::Chained { return std::get(lhs) != std::get(rhs); } - using DerefFunc = DerefType (*)(IterTupType&); - using ArrowFunc = ArrowType (*)(IterTupType&); + using DerefFunc = DerefType (*)(const IterTupType&); + using ArrowFunc = ArrowType (*)(const IterTupType&); using IncFunc = void (*)(IterTupType&); using NeqFunc = bool (*)(const IterTupType&, const IterTupType&); @@ -137,11 +137,11 @@ class iter::impl::Chained { check_for_end_and_adjust(); } - decltype(auto) operator*() { + decltype(auto) operator*() const { return IterData::derefers[index_](iters_); } - decltype(auto) operator-> () { + decltype(auto) operator->() const { return IterData::arrowers[index_](iters_); } @@ -161,7 +161,7 @@ class iter::impl::Chained { bool operator!=(const Iterator& other) const { return index_ != other.index_ || (index_ != sizeof...(Is) - && IterData::neq_comparers[index_](iters_, other.iters_)); + && IterData::neq_comparers[index_](iters_, other.iters_)); } bool operator==(const Iterator& other) const { @@ -278,11 +278,11 @@ class iter::impl::ChainedFromIterable { return !(*this != other); } - iterator_deref> operator*() { + iterator_deref> operator*() const { return **sub_iter_p_; } - iterator_arrow> operator->() { + iterator_arrow> operator->() const { return apply_arrow(*sub_iter_p_); } }; diff --git a/chunked.hpp b/chunked.hpp index 1d03b6e1..e4ea39e9 100644 --- a/chunked.hpp +++ b/chunked.hpp @@ -104,11 +104,11 @@ class iter::impl::Chunker { && (done() || !(sub_iter_ != other.sub_iter_)); } - DerefVec& operator*() { + DerefVec& operator*() const { return *chunk_; } - DerefVec* operator->() { + DerefVec* operator->() const { return chunk_.get(); } }; diff --git a/combinations.hpp b/combinations.hpp index 7572142c..96eef566 100644 --- a/combinations.hpp +++ b/combinations.hpp @@ -43,7 +43,7 @@ class iter::impl::Combinator { friend class Iterator; constexpr static const int COMPLETE = -1; std::remove_reference_t* container_p_; - CombIteratorDeref indices_; + mutable CombIteratorDeref indices_; int steps_{}; public: @@ -73,11 +73,11 @@ class iter::impl::Combinator { } } - CombIteratorDeref& operator*() { + CombIteratorDeref& operator*() const { return indices_; } - CombIteratorDeref* operator->() { + CombIteratorDeref* operator->() const { return &indices_; } diff --git a/combinations_with_replacement.hpp b/combinations_with_replacement.hpp index b7e20d7d..61b5f007 100644 --- a/combinations_with_replacement.hpp +++ b/combinations_with_replacement.hpp @@ -43,7 +43,7 @@ class iter::impl::CombinatorWithReplacement { friend class Iterator; constexpr static const int COMPLETE = -1; std::remove_reference_t* container_p_; - CombIteratorDeref indices_; + mutable CombIteratorDeref indices_; int steps_; public: @@ -60,11 +60,11 @@ class iter::impl::CombinatorWithReplacement { ? 0 : COMPLETE} {} - CombIteratorDeref& operator*() { + CombIteratorDeref& operator*() const { return indices_; } - CombIteratorDeref* operator->() { + CombIteratorDeref* operator->() const { return &indices_; } diff --git a/compress.hpp b/compress.hpp index f4606ef8..08cc1055 100644 --- a/compress.hpp +++ b/compress.hpp @@ -73,11 +73,11 @@ class iter::impl::Compressed { skip_failures(); } - iterator_deref operator*() { + iterator_deref operator*() const { return *sub_iter_; } - iterator_arrow operator->() { + iterator_arrow operator->() const { return apply_arrow(sub_iter_); } diff --git a/cycle.hpp b/cycle.hpp index 1d3ecaef..ad8ee789 100644 --- a/cycle.hpp +++ b/cycle.hpp @@ -52,11 +52,11 @@ class iter::impl::Cycler { sub_begin_{sub_iter}, sub_end_{std::move(sub_end)} {} - iterator_deref operator*() { + iterator_deref operator*() const { return *sub_iter_; } - iterator_arrow operator->() { + iterator_arrow operator->() const { return apply_arrow(sub_iter_); } diff --git a/dropwhile.hpp b/dropwhile.hpp index dc1874b0..a11d8f04 100644 --- a/dropwhile.hpp +++ b/dropwhile.hpp @@ -79,12 +79,12 @@ class iter::impl::Dropper { sub_end_{std::move(sub_end)}, filter_func_(&filter_func) {} - typename Holder::reference operator*() { + typename Holder::reference operator*() const { init_if_first_use(); return item_.get(); } - typename Holder::pointer operator->() { + typename Holder::pointer operator->() const { init_if_first_use(); return item_.get_ptr(); } diff --git a/enumerate.hpp b/enumerate.hpp index e2dda12b..00390ea2 100644 --- a/enumerate.hpp +++ b/enumerate.hpp @@ -85,11 +85,11 @@ class iter::impl::Enumerable { Iterator(IteratorWrapper&& sub_iter, Index start) : sub_iter_{std::move(sub_iter)}, index_{start} {} - IterYield operator*() { + IterYield operator*() const { return {index_, *sub_iter_}; } - ArrowProxy> operator->() { + ArrowProxy> operator->() const { return {**this}; } diff --git a/filter.hpp b/filter.hpp index 2a632be2..409fc114 100644 --- a/filter.hpp +++ b/filter.hpp @@ -93,12 +93,12 @@ class iter::impl::Filtered { sub_end_{std::move(sub_end)}, filter_func_(&filter_func) {} - typename Holder::reference operator*() { + typename Holder::reference operator*() const { init_if_first_use(); return item_.get(); } - typename Holder::pointer operator->() { + typename Holder::pointer operator->() const { init_if_first_use(); return item_.get_ptr(); } diff --git a/groupby.hpp b/groupby.hpp index cac82426..daaebd5e 100644 --- a/groupby.hpp +++ b/groupby.hpp @@ -61,7 +61,7 @@ class iter::impl::GroupProducer { IteratorWrapper sub_end_; Holder item_; KeyFunc* key_func_; - std::optional> current_key_group_pair_; + mutable std::optional> current_key_group_pair_; public: using iterator_category = std::input_iterator_tag; @@ -77,6 +77,7 @@ class iter::impl::GroupProducer { key_func_(&key_func) { if (sub_iter_ != sub_end_) { item_.reset(*sub_iter_); + set_key_group_pair(); } } @@ -84,7 +85,9 @@ class iter::impl::GroupProducer { : sub_iter_{other.sub_iter_}, sub_end_{other.sub_end_}, item_{other.item_}, - key_func_{other.key_func_} {} + key_func_{other.key_func_} { + set_key_group_pair(); + } Iterator& operator=(const Iterator& other) { if (this == &other) { @@ -95,6 +98,7 @@ class iter::impl::GroupProducer { item_ = other.item_; key_func_ = other.key_func_; current_key_group_pair_.reset(); + set_key_group_pair(); return *this; } @@ -103,13 +107,11 @@ class iter::impl::GroupProducer { // NOTE the implicitly generated move constructor would // be wrong - KeyGroupPair& operator*() { - set_key_group_pair(); + KeyGroupPair& operator*() const { return *current_key_group_pair_; } - KeyGroupPair* operator->() { - set_key_group_pair(); + KeyGroupPair* operator->() const { return &*current_key_group_pair_; } @@ -118,6 +120,7 @@ class iter::impl::GroupProducer { set_key_group_pair(); } current_key_group_pair_.reset(); + set_key_group_pair(); return *this; } @@ -163,7 +166,7 @@ class iter::impl::GroupProducer { } void set_key_group_pair() { - if (!current_key_group_pair_) { + if (!current_key_group_pair_ && item_) { current_key_group_pair_.emplace(std::invoke(*key_func_, item_.get()), Group{*this, next_key()}); } @@ -251,11 +254,11 @@ class iter::impl::GroupProducer { return ret; } - iterator_deref operator*() { + iterator_deref operator*() const { return group_p_->owner_.get(); } - typename Holder::pointer operator->() { + typename Holder::pointer operator->() const { return group_p_->owner_.get_ptr(); } }; diff --git a/internal/iterator_wrapper.hpp b/internal/iterator_wrapper.hpp index f068da97..929e0b3a 100644 --- a/internal/iterator_wrapper.hpp +++ b/internal/iterator_wrapper.hpp @@ -4,6 +4,7 @@ #include #include #include + #include "iterbase.hpp" namespace iter { diff --git a/permutations.hpp b/permutations.hpp index e10872d4..0aee740c 100644 --- a/permutations.hpp +++ b/permutations.hpp @@ -48,7 +48,7 @@ class iter::impl::Permuter { return *lhs < *rhs; } - Permutable working_set_; + mutable Permutable working_set_; int steps_{}; public: @@ -72,11 +72,11 @@ class iter::impl::Permuter { cmp_iters); } - Permutable& operator*() { + Permutable& operator*() const { return working_set_; } - Permutable* operator->() { + Permutable* operator->() const { return &working_set_; } diff --git a/powerset.hpp b/powerset.hpp index 3439069d..34bc0def 100644 --- a/powerset.hpp +++ b/powerset.hpp @@ -82,11 +82,11 @@ class iter::impl::Powersetter { return ret; } - iterator_deref> operator*() { + iterator_deref> operator*() const { return *comb_iter_; } - iterator_arrow> operator->() { + iterator_arrow> operator->() const { apply_arrow(comb_iter_); } diff --git a/product.hpp b/product.hpp index 7de5727a..493e0e64 100644 --- a/product.hpp +++ b/product.hpp @@ -144,11 +144,11 @@ class iter::impl::Productor { return !(*this != other); } - TupleDeref operator*() { + TupleDeref operator*() const { return {(*std::get(iters_))...}; } - auto operator->() -> ArrowProxy { + auto operator->() const -> ArrowProxy { return {**this}; } }; diff --git a/reversed.hpp b/reversed.hpp index 3b914bb1..b6264470 100644 --- a/reversed.hpp +++ b/reversed.hpp @@ -87,11 +87,11 @@ class iter::impl::Reverser { Iterator(ReverseIteratorWrapper&& sub_iter) : sub_iter_{std::move(sub_iter)} {} - reverse_iterator_deref operator*() { + reverse_iterator_deref operator*() const { return *sub_iter_; } - reverse_iterator_arrow operator->() { + reverse_iterator_arrow operator->() const { return apply_arrow(sub_iter_); } diff --git a/slice.hpp b/slice.hpp index a926ecbb..4b249497 100644 --- a/slice.hpp +++ b/slice.hpp @@ -62,11 +62,11 @@ class iter::impl::Sliced { stop_{stop}, step_{step} {} - iterator_deref operator*() { + iterator_deref operator*() const { return *sub_iter_; } - iterator_arrow operator->() { + iterator_arrow operator->() const { return apply_arrow(sub_iter_); } diff --git a/sliding_window.hpp b/sliding_window.hpp index d4ab6cbc..9a58d577 100644 --- a/sliding_window.hpp +++ b/sliding_window.hpp @@ -76,11 +76,11 @@ class iter::impl::WindowSlider { return !(*this != other); } - DerefVec& operator*() { + DerefVec& operator*() const { return *window_; } - DerefVec* operator->() { + DerefVec* operator->() const { return window_.get(); } diff --git a/starmap.hpp b/starmap.hpp index fbeaad68..0871fc36 100644 --- a/starmap.hpp +++ b/starmap.hpp @@ -84,11 +84,11 @@ class iter::impl::StarMapper { return ret; } - decltype(auto) operator*() { + decltype(auto) operator*() const { return std::apply(*func_, *sub_iter_); } - auto operator->() -> ArrowProxy { + auto operator->() const -> ArrowProxy { return {**this}; } }; @@ -172,11 +172,11 @@ class iter::impl::TupleStarMapper { Iterator(Func& f, TupTypeT& t, std::size_t i) : func_{&f}, tup_{&t}, index_{i} {} - decltype(auto) operator*() { + decltype(auto) operator*() const { return IteratorData::callers[index_](*func_, *tup_); } - auto operator->() { + auto operator->() const { return ArrowProxy{**this}; } diff --git a/takewhile.hpp b/takewhile.hpp index a04d86b2..d1448835 100644 --- a/takewhile.hpp +++ b/takewhile.hpp @@ -81,12 +81,12 @@ class iter::impl::Taker { sub_end_{std::move(sub_end)}, filter_func_(&filter_func) {} - typename Holder::reference operator*() { + typename Holder::reference operator*() const { init_if_first_use(); return item_.get(); } - typename Holder::pointer operator->() { + typename Holder::pointer operator->() const { init_if_first_use(); return item_.get_ptr(); } diff --git a/test/helpers.hpp b/test/helpers.hpp index 93112519..13d30437 100644 --- a/test/helpers.hpp +++ b/test/helpers.hpp @@ -65,7 +65,7 @@ namespace itertest { class Iterator { private: int i; - bool was_incremented = true; + mutable bool was_incremented = true; public: Iterator(int n) : i{n} {} @@ -76,7 +76,7 @@ namespace itertest { return *this; } - int operator*() { + int operator*() const { if (!this->was_incremented) { throw DoubleDereferenceError{}; } @@ -175,7 +175,7 @@ namespace itertest { return *this; } - U& operator*() { + U& operator*() const { return *this->p; } }; diff --git a/zip.hpp b/zip.hpp index 98bfd3c4..22cbafd6 100644 --- a/zip.hpp +++ b/zip.hpp @@ -87,11 +87,11 @@ class iter::impl::Zipped { return !(*this != other); } - TupleDeref operator*() { + TupleDeref operator*() const { return {(*std::get(iters_))...}; } - auto operator->() -> ArrowProxy { + auto operator->() const -> ArrowProxy { return {**this}; } }; diff --git a/zip_longest.hpp b/zip_longest.hpp index 238ec569..00fd5de1 100644 --- a/zip_longest.hpp +++ b/zip_longest.hpp @@ -97,13 +97,13 @@ class iter::impl::ZippedLongest { return !(*this != other); } - ZipIterDeref operator*() { + ZipIterDeref operator*() const { return {((std::get(iters_) != std::get(ends_)) ? OptTempl{*std::get(iters_)} : OptTempl{})...}; } - auto operator-> () -> ArrowProxy { + auto operator-> () const -> ArrowProxy { return {**this}; } }; From 15c96e1f582055397826d8d4002eab9b557f03fa Mon Sep 17 00:00:00 2001 From: Thomas Weidner Date: Sat, 25 Feb 2023 14:35:00 +0100 Subject: [PATCH 2/2] Allow `ArrowHelper` to work with all pointer types. Just specializing on `T*` misses cv-qualified pointer types like `int *const`. This change uses `std::is_pointer_v` instead to determine whether a type is a pointer. --- internal/iterbase.hpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/iterbase.hpp b/internal/iterbase.hpp index 6f654828..21472536 100644 --- a/internal/iterbase.hpp +++ b/internal/iterbase.hpp @@ -144,9 +144,9 @@ namespace iter { }; template - struct ArrowHelper { - using type = T*; - constexpr type operator()(T* t) const noexcept { + struct ArrowHelper>> { + using type = T; + constexpr type operator()(T t) const noexcept { return t; } };