On Tue, 19 Aug 2025 at 16:24, Patrick Palka <ppa...@redhat.com> wrote:
>
> On Wed, 16 Jul 2025, Tomasz Kaminski wrote:
>
> >
> >
> > On Tue, Jul 15, 2025 at 6:13 PM Patrick Palka <ppa...@redhat.com> wrote:
> > Tested on x86_64-pc-linux-gnu, does this look OK for trunk only
> > (since it impacts ABI)?
> >
> > Changes in v2:
> >
> > - Condition on forward_iterator instead of default_initializable.
> >
> > -- >8 --
> >
> > LWG 3569 adjusted join_view's iterator specification to handle non
> > default-constructible iterators by wrapping the corresponding data
> > member
> > in std::optional, which we followed suit in r13-2649-g7aa80c82ecf3a3.
> >
> > But this wrapping is unnecessary for iterators that are already
> > default-constructible. Rather than unconditionally using
> > std::optional
> > here, which introduces time/space overhead, this patch conditionalizes
> > our LWG 3569 changes on the iterator in question being non-forward
> > (and
> > thus non default-constructible). We check forwardness instead of
> > default-constructibility in order to accomodate input-only iterators
> > whose default constructor might be underconstrained.
> >
> > I would rephrase that sentence a bit, to:
> > We check forwardness instead of
> > default-constructibility in order to accomodate input-only iterators that
> > satisfies but does not model default_initializable, e.g. whose default
> > constructor
> > is underconstrained.
> >
> > Outside of that this LGTM.
>
> Thanks. Is it OK to push (with that sentence rephrased)?
OK for trunk
>
> >
> > libstdc++-v3/ChangeLog:
> >
> > * include/std/ranges (join_view::_Iterator::_M_satisfy):
> > Adjust to handle non-std::optional _M_inner as per before LWG
> > 3569.
> > (join_view::_Iterator::_M_get_inner): New.
> > (join_view::_Iterator::_M_inner): Don't wrap in std::optional
> > if
> > the iterator is forward. Initialize.
> > (join_view::_Iterator::operator*): Use _M_get_inner instead
> > of *_M_inner.
> > (join_view::_Iterator::operator++): Likewise.
> > (join_view::_Iterator::iter_move): Likewise.
> > (join_view::_Iterator::iter_swap): Likewise.
> > ---
> > libstdc++-v3/include/std/ranges | 49
> > +++++++++++++++++++++++++--------
> > 1 file changed, 37 insertions(+), 12 deletions(-)
> >
> > diff --git a/libstdc++-v3/include/std/ranges
> > b/libstdc++-v3/include/std/ranges
> > index efe62969d657..c9dc25ee52ef 100644
> > --- a/libstdc++-v3/include/std/ranges
> > +++ b/libstdc++-v3/include/std/ranges
> > @@ -2971,7 +2971,12 @@ namespace views::__adaptor
> > }
> >
> > if constexpr (_S_ref_is_glvalue)
> > - _M_inner.reset();
> > + {
> > + if constexpr (forward_iterator<_Inner_iter>)
> > + _M_inner = _Inner_iter();
> > + else
> > + _M_inner.reset();
> > + }
> > }
> >
> > static constexpr auto
> > @@ -3011,6 +3016,24 @@ namespace views::__adaptor
> > return *_M_parent->_M_outer;
> > }
> >
> > + constexpr _Inner_iter&
> > + _M_get_inner()
> > + {
> > + if constexpr (forward_iterator<_Inner_iter>)
> > + return _M_inner;
> > + else
> > + return *_M_inner;
> > + }
> > +
> > + constexpr const _Inner_iter&
> > + _M_get_inner() const
> > + {
> > + if constexpr (forward_iterator<_Inner_iter>)
> > + return _M_inner;
> > + else
> > + return *_M_inner;
> > + }
> > +
> > constexpr
> > _Iterator(_Parent* __parent, _Outer_iter __outer) requires
> > forward_range<_Base>
> > : _M_outer(std::move(__outer)), _M_parent(__parent)
> > @@ -3024,7 +3047,9 @@ namespace views::__adaptor
> > [[no_unique_address]]
> > __detail::__maybe_present_t<forward_range<_Base>,
> > _Outer_iter> _M_outer
> > = decltype(_M_outer)();
> > - optional<_Inner_iter> _M_inner;
> > + __conditional_t<forward_iterator<_Inner_iter>,
> > + _Inner_iter, optional<_Inner_iter>> _M_inner
> > + = decltype(_M_inner)();
> > _Parent* _M_parent = nullptr;
> >
> > public:
> > @@ -3048,7 +3073,7 @@ namespace views::__adaptor
> >
> > constexpr decltype(auto)
> > operator*() const
> > - { return **_M_inner; }
> > + { return *_M_get_inner(); }
> >
> > // _GLIBCXX_RESOLVE_LIB_DEFECTS
> > // 3500. join_view::iterator::operator->() is bogus
> > @@ -3056,7 +3081,7 @@ namespace views::__adaptor
> > operator->() const
> > requires __detail::__has_arrow<_Inner_iter>
> > && copyable<_Inner_iter>
> > - { return *_M_inner; }
> > + { return _M_get_inner(); }
> >
> > constexpr _Iterator&
> > operator++()
> > @@ -3067,7 +3092,7 @@ namespace views::__adaptor
> > else
> > return *_M_parent->_M_inner;
> > }();
> > - if (++*_M_inner == ranges::end(__inner_range))
> > + if (++_M_get_inner() == ranges::end(__inner_range))
> > {
> > ++_M_get_outer();
> > _M_satisfy();
> > @@ -3097,9 +3122,9 @@ namespace views::__adaptor
> > {
> > if (_M_outer == ranges::end(_M_parent->_M_base))
> > _M_inner =
> > ranges::end(__detail::__as_lvalue(*--_M_outer));
> > - while (*_M_inner ==
> > ranges::begin(__detail::__as_lvalue(*_M_outer)))
> > - *_M_inner =
> > ranges::end(__detail::__as_lvalue(*--_M_outer));
> > - --*_M_inner;
> > + while (_M_get_inner() ==
> > ranges::begin(__detail::__as_lvalue(*_M_outer)))
> > + _M_get_inner() =
> > ranges::end(__detail::__as_lvalue(*--_M_outer));
> > + --_M_get_inner();
> > return *this;
> > }
> >
> > @@ -3126,14 +3151,14 @@ namespace views::__adaptor
> >
> > friend constexpr decltype(auto)
> > iter_move(const _Iterator& __i)
> > - noexcept(noexcept(ranges::iter_move(*__i._M_inner)))
> > - { return ranges::iter_move(*__i._M_inner); }
> > + noexcept(noexcept(ranges::iter_move(__i._M_get_inner())))
> > + { return ranges::iter_move(__i._M_get_inner()); }
> >
> > friend constexpr void
> > iter_swap(const _Iterator& __x, const _Iterator& __y)
> > - noexcept(noexcept(ranges::iter_swap(*__x._M_inner,
> > *__y._M_inner)))
> > + noexcept(noexcept(ranges::iter_swap(__x._M_get_inner(),
> > __y._M_get_inner())))
> > requires indirectly_swappable<_Inner_iter>
> > - { return ranges::iter_swap(*__x._M_inner, *__y._M_inner); }
> > + { return ranges::iter_swap(__x._M_get_inner(),
> > __y._M_get_inner()); }
> >
> > friend _Iterator<!_Const>;
> > template<bool> friend struct _Sentinel;
> > --
> > 2.50.1.271.gd30e120486
> >
> >
> >
