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.
> -- >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
>
>
