On Tue, Jul 15, 2025 at 5:43 PM Patrick Palka <ppa...@redhat.com> wrote:
> On Tue, 15 Jul 2025, Tomasz Kaminski wrote:
>
> >
> >
> > On Tue, Jul 15, 2025 at 3:55 PM Patrick Palka <ppa...@redhat.com> wrote:
> > On Tue, 15 Jul 2025, Tomasz Kaminski wrote:
> >
> > > On Tue, Jul 15, 2025 at 5:51 AM Patrick Palka <ppa...@redhat.com>
> wrote:
> > > Tested on x86_64-pc-linux-gnu, does this look OK for trunk
> only
> > > (since it impacts ABI)?
> > >
> > > In theory an Iterator that meets all semantic requirements of
> the input_iterator
> > > concept, could provide a default constructor that is
> unconstrained, but ill-formed
> > > when invoked. This can be easily done accidentally, by having a
> default member initializer.
> > >
> > > #include <concepts>
> > >
> > > struct NoDefault
> > > { NoDefault(int); };
> > >
> > > template<typename T>
> > > struct Iterator {
> > > T x = T();
> > > };
> > >
> > > static_assert(std::default_initializable<Iterator<NoDefault>>);
> > >
> > > Default member initializers are not in immediate context, and
> checking "std::default_initializable"
> > > is ill-formed. clang emits error here:
> https://godbolt.org/z/EafKn6h16
> > >
> > > You can however, do this optimization for forward_iterator. The
> difference here is that user-defined
> > > iterators provides iterator_category/iterator_concept that maps
> to forward_iterator_tag or stronger,
> > > so we can check default_initializable.
> >
> > Good point... But it seems this is not only an issue in join_view
> (with
> > this patch), we already require elsewhere in <ranges> that the
> default
> > ctor of an input iterator is properly constrained:
> >
> > filter_view, transform_view, elements_view, stride_view,
> enumerate_view,
> > to_input_view (and perhaps iota_view also counts)
> >
> > Could you please elaborate? I understand that for this view, they
> default constructor
> > requires that iterator is default_initializable, but if you are never
> calling this constructor,
> > the view will function correctly for my not-properly constrained
> iterator.
> >
> > However, for this case the standard does not impose
> default_initializable requirement
> > for iterator, when we are incrementing it.
>
> I might be confused, is supporting such underconstrained iterators
> a QoI issue or a correctness issue?
>
> If it's QoI, note that before P2325R3 (approved June 2021) even C++20
> input iterators were required to satisfy default_initializable, so I
> reckon it's quite rare to see C++20 input iterator written after that
> with an underconstrained default ctor. I'm not sure supporting them is
> worth the tradeoff of pessimizing join_view for input-only iterators.
>
> If it's a correctness issue, I definitely agree with your changes :)
>
I think that this is correctness issue, the LWG3569 removed the
default_initializable
requires on the inner_iterator for join_view, and this has two aspect:
* we do not need to satisfy default_initializable -
default_initializable<IT> returns true
* we do not need to model default_initializable - semantics requirement, in
particular
default_initializable<IT> not lying, i.e. when it is true, the
constructor is well-formed
So, I believe the standard requires that no properly constrained
input_iterator to work
now, i.e. we cannot add a default_initializable check. Adding a
default_initializable check,
would for me mean partially reverting the issue implementation.
The corresponding wording is here:
https://eel.is/c++draft/res.on.requirements#2
> If the validity or meaning of a program depends on whether a sequence of
template arguments
models a concept, and the concept is satisfied but not modeled, the
program is ill-formed, no diagnostic required.
<https://eel.is/c++draft/res.on.requirements#2.sentence-1>
That means if we say requires or Constrains, you cannot have a lying
concept.
However, despite lacking explicit wording, we believe that implementations
are allowed
to promote concepts, i.e. check forward_iterator even if functions require
only input_iterator.
I saw your updated patch, I will check it tomorrow.
Hope this makes sense,
Tomasz
>
> >
> >
> > And so these views would break similarly if the default ctor is
> > underconstrained IIUC. I don't see why we'd want to start caring
> about
> > such iterators in join_view, if other fundamental views already
> don't?
> >
> > >
> > >
> > >
> > > -- >8 --
> > >
> > > LWG 3569 adjusted join_view's iterator to handle adapting
> > > non-default-constructible (input) iterators by wrapping the
> > > corresponding data member with std::optional, and 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-default-constructible.
> > >
> > >
> > > 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 already default constructible.
> 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..799fa7611ce2 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
> (default_initializable<_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
> (default_initializable<_Inner_iter>)
> > > + return _M_inner;
> > > + else
> > > + return *_M_inner;
> > > + }
> > > +
> > > + constexpr const _Inner_iter&
> > > + _M_get_inner() const
> > > + {
> > > + if constexpr
> (default_initializable<_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<default_initializable<_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
> > >
> > >
> > >
> >
> >
> >
