[PATCH 1/2] libstdc++: Implement P3725R3 Filter View Extensions for Safer Use
Jonathan Wakely
jwakely@redhat.com
Wed Apr 1 18:18:08 GMT 2026
On Wed, 1 Apr 2026 at 18:35, Patrick Palka <ppalka@redhat.com> wrote:
>
> On Wed, 1 Apr 2026, Jonathan Wakely wrote:
>
> > On Wed, 1 Apr 2026 at 15:50, Patrick Palka <ppalka@redhat.com> wrote:
> > >
> > > On Tue, 31 Mar 2026, Jonathan Wakely wrote:
> > >
> > > > On Tue, 31 Mar 2026 at 19:43, Patrick Palka <ppalka@redhat.com> wrote:
> > > > >
> > > > > Tested on x86_64-pc-linux-gnu, does this look OK for trunk and
> > > > > eventually backports?
> > > >
> > > > OK for trunk, yes. Thanks for doing this so quickly!
> > >
> > > Thanks for the review.
> > >
> > > >
> > > > For the backports we should either not do it, or add separate
> > > > _CIterator and _CSentinel instead of changing the iterator and
> > > > sentinel into templates, so that we don't change the ABI of the
> > > > existing non-const filter_view::begin() and filter_view::end()
> > > > members.
> > > > Otherwise we'd change the return types of those functions in a dot
> > > > release (15.3 and 14.4) which is not what people expect/want when
> > > > updating within a stable release.
> > >
> > > Ah, makes sense. Agreed in principle, but I was struggling to think of
> > > a concrete scenario that'd actually cause user breakage with this change,
> > > (i.e. changing the mangling of a user-visible implementation-detail type
> > > that is nonetheless layout-compatible with the original type). I thought
> > > layout-compatibility would be enough to avoid breakage, but that's not
> > > true.
> > >
> > > If a user-defined function depends on the type of the filter_view
> > > iterator, e.g.:
> > >
> > > inline void f(auto& it) { }
> > > ...
> > > auto v = r | views::filter(p);
> > > f(v.begin());
> > >
> > > and one TU with such an f call was compiled before the change, and
> > > another such TU was compiled after the change, then the two TUs now
> > > refer to logically distinct functions, with different manglings and
> > > addresses. This could be problematic if user code stores their address
> > > across TUs and later compares it. Also any static local variables
> > > within f won't be merged by the linker and we'll end up with two copies
> > > thereof. Unlikely, but theoretically possible breakage, so still
> > > must be avoided on a release branch.
> > >
> > > I think we could avoid changing the mangling of the original _Iterator
> > > in a relatively concise way with:
> > >
> > > struct _Iterator : _GIterator<_Iterator, false>
> > > { using _GIterator<false>::_GIterator; };
> > >
> > > where _GIterator is a CRTP version of the new generic iterator class
> > > template. The mangling of _Iterator's member functions would still
> > > change but I believe that's fine because user code can't take the
> > > address of such standard member functions (and so the mangling of
> > > user-defined functions can't depend on _Iterator's member functions).
> >
> > I think you'd need to override operator++ and operator-- because the
> > ones in the base class will return the base class, not the derived
> > class :-(
>
> That's where CRTP comes in handy, the following restores the original
> _Iterator mangling with minimal code duplication and passes testing:
Ah yes, of course. For some reason I glossed over the "CRTP" in your last mail.
This looks like a good solution for the branches, which avoids any
change in mangled name for the _Iterator and _Sentinel cases.
>
> -- >8 --
>
> libstdc++-v3/ChangeLog:
>
> * include/std/ranges (views::__adaptor::filter_view::_Iterator):
> Rename template to _GIterator and add a _Derived CRTP parameter.
> (views::__adaptor::filter_view::_Sentinel): Rename template to
> _GSentinel and add a _Derived CRTP parameter.
> (views::__adaptor::filter_view::_Iterator): Reintroduce as a
> non-template class inheriting from _GIterator<_Iterator, false>.
> (views::__adaptor::filter_view::_CIterator): New non-template
> class inheriting from _GIterator<_CIterator, true>.
> (views::__adaptor::filter_view::_Sentinel): Reintroduce as a
> non-template class inheriting from _GSentinel<_Sentinel, false>.
> (views::__adaptor::filter_view::_CSentinel): New non-template
> class inheriting from _GSentinel<_CSentinel, true>.
> (views::__adaptor::filter_view::begin): Update return types to use
> _Iterator or _CIterator.
> (views::__adaptor::filter_view::end): Update return types to use
> _Iterator, _Sentinel, or _CSentinel.
>
> libstdc++-v3/include/std/ranges | 89 +++++++++++++++++++++++------------------
> 1 file changed, 50 insertions(+), 39 deletions(-)
>
> diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges
> index 0aa4191e04f6..da91975e5d1e 100644
> --- a/libstdc++-v3/include/std/ranges
> +++ b/libstdc++-v3/include/std/ranges
> @@ -1727,11 +1727,8 @@ namespace views::__adaptor
> class filter_view : public view_interface<filter_view<_Vp, _Pred>>
> {
> private:
> - template<bool _Const>
> - struct _Sentinel;
> -
> - template<bool _Const>
> - struct _Iterator : __detail::__filter_view_iter_cat<_Vp>
> + template<typename _Derived, bool _Const>
> + struct _GIterator : __detail::__filter_view_iter_cat<_Vp>
> {
> private:
> static constexpr auto
> @@ -1748,7 +1745,7 @@ namespace views::__adaptor
> }
>
> friend filter_view;
> - friend _Iterator<!_Const>;
> + template<typename, bool> friend struct _GIterator;
>
> using _Parent = __maybe_const_t<_Const, filter_view>;
> using _Base = __maybe_const_t<_Const, _Vp>;
> @@ -1763,20 +1760,21 @@ namespace views::__adaptor
> using value_type = range_value_t<_Base>;
> using difference_type = range_difference_t<_Base>;
>
> - _Iterator() requires default_initializable<_Base_iter> = default;
> + _GIterator() requires default_initializable<_Base_iter> = default;
>
> constexpr
> - _Iterator(_Parent* __parent, _Base_iter __current)
> + _GIterator(_Parent* __parent, _Base_iter __current)
> : _M_current(std::move(__current)),
> _M_parent(__parent)
> { }
>
> - constexpr
> - _Iterator(_Iterator<!_Const> __i)
> + template<typename _Derived2>
> requires _Const && convertible_to<iterator_t<_Vp>, iterator_t<_Base>>
> - : _M_current(std::move(__i._M_current)),
> - _M_parent(std::move(__i._M_parent))
> - { }
> + constexpr
> + _GIterator(_GIterator<_Derived2, !_Const> __i)
> + : _M_current(std::move(__i._M_current)),
> + _M_parent(std::move(__i._M_parent))
> + { }
>
> constexpr const _Base_iter&
> base() const & noexcept
> @@ -1796,20 +1794,20 @@ namespace views::__adaptor
> && copyable<_Base_iter>
> { return _M_current; }
>
> - constexpr _Iterator&
> + constexpr _Derived&
> operator++()
> {
> _M_current = ranges::find_if(std::move(++_M_current),
> ranges::end(_M_parent->_M_base),
> std::ref(*_M_parent->_M_pred));
> - return *this;
> + return static_cast<_Derived&>(*this);
> }
>
> constexpr void
> operator++(int)
> { ++*this; }
>
> - constexpr _Iterator
> + constexpr _Derived
> operator++(int) requires forward_range<_Base>
> {
> auto __tmp = *this;
> @@ -1817,16 +1815,16 @@ namespace views::__adaptor
> return __tmp;
> }
>
> - constexpr _Iterator&
> + constexpr _Derived&
> operator--() requires bidirectional_range<_Base>
> {
> do
> --_M_current;
> while (!std::__invoke(*_M_parent->_M_pred, *_M_current));
> - return *this;
> + return static_cast<_Derived&>(*this);
> }
>
> - constexpr _Iterator
> + constexpr _Derived
> operator--(int) requires bidirectional_range<_Base>
> {
> auto __tmp = *this;
> @@ -1835,58 +1833,71 @@ namespace views::__adaptor
> }
>
> friend constexpr bool
> - operator==(const _Iterator& __x, const _Iterator& __y)
> + operator==(const _GIterator& __x, const _GIterator& __y)
> requires equality_comparable<_Base_iter>
> { return __x._M_current == __y._M_current; }
>
> friend constexpr range_rvalue_reference_t<_Base>
> - iter_move(const _Iterator& __i)
> + iter_move(const _GIterator& __i)
> noexcept(noexcept(ranges::iter_move(__i._M_current)))
> { return ranges::iter_move(__i._M_current); }
>
> friend constexpr void
> - iter_swap(const _Iterator& __x, const _Iterator& __y)
> + iter_swap(const _GIterator& __x, const _GIterator& __y)
> noexcept(noexcept(ranges::iter_swap(__x._M_current, __y._M_current)))
> requires indirectly_swappable<_Base_iter>
> { ranges::iter_swap(__x._M_current, __y._M_current); }
> };
>
> - template<bool _Const>
> - struct _Sentinel
> + template<typename _Derived, bool _Const>
> + struct _GSentinel
> {
> private:
> using _Parent = __maybe_const_t<_Const, filter_view>;
> using _Base = __maybe_const_t<_Const, _Vp>;
> sentinel_t<_Base> _M_end = sentinel_t<_Base>();
>
> - friend _Sentinel<!_Const>;
> + template<typename, bool> friend struct _GSentinel;
>
> public:
> - _Sentinel() = default;
> + _GSentinel() = default;
>
> constexpr explicit
> - _Sentinel(_Parent* __parent)
> + _GSentinel(_Parent* __parent)
> : _M_end(ranges::end(__parent->_M_base))
> { }
>
> - constexpr
> - _Sentinel(_Sentinel<!_Const> __i)
> + template<typename _Derived2>
> requires _Const && convertible_to<sentinel_t<_Vp>, sentinel_t<_Base>>
> - : _M_end(std::move(__i._M_end))
> - { }
> + constexpr
> + _GSentinel(_GSentinel<_Derived2, !_Const> __i)
> + : _M_end(std::move(__i._M_end))
> + { }
>
> constexpr sentinel_t<_Base>
> base() const
> { return _M_end; }
>
> - template<bool _Const2>
> + template<typename _Derived2, bool _Const2>
> requires sentinel_for<sentinel_t<_Base>,
> iterator_t<__maybe_const_t<_Const2, _Vp>>>
> friend constexpr bool
> - operator==(const _Iterator<_Const2>& __x, const _Sentinel& __y)
> + operator==(const _GIterator<_Derived2, _Const2>& __x, const _GSentinel& __y)
> { return __x._M_current == __y._M_end; }
> };
>
> + struct _Iterator : _GIterator<_Iterator, false>
> + { using _GIterator<_Iterator, false>::_GIterator; };
> +
> + struct _CIterator : _GIterator<_CIterator, true>
> + { using _GIterator<_CIterator, true>::_GIterator; };
> +
> + struct _Sentinel : _GSentinel<_Sentinel, false>
> + { using _GSentinel<_Sentinel, false>::_GSentinel; };
> +
> + struct _CSentinel : _GSentinel<_CSentinel, true>
> + { using _GSentinel<_CSentinel, true>::_GSentinel; };
> +
> _Vp _M_base = _Vp();
> [[no_unique_address]] __detail::__box<_Pred> _M_pred;
> [[no_unique_address]] __detail::_CachedPosition<_Vp> _M_cached_begin;
> @@ -1913,7 +1924,7 @@ namespace views::__adaptor
> pred() const
> { return *_M_pred; }
>
> - constexpr _Iterator<false>
> + constexpr _Iterator
> begin()
> {
> if (_M_cached_begin._M_has_value())
> @@ -1927,7 +1938,7 @@ namespace views::__adaptor
> return {this, std::move(__it)};
> }
>
> - constexpr _Iterator<true>
> + constexpr _CIterator
> begin() const
> requires (input_range<const _Vp> && !forward_range<const _Vp>
> && indirect_unary_predicate<const _Pred, iterator_t<const _Vp>>)
> @@ -1943,16 +1954,16 @@ namespace views::__adaptor
> end()
> {
> if constexpr (common_range<_Vp>)
> - return _Iterator<false>{this, ranges::end(_M_base)};
> + return _Iterator{this, ranges::end(_M_base)};
> else
> - return _Sentinel<false>{this};
> + return _Sentinel{this};
> }
>
> - constexpr _Sentinel<true>
> + constexpr _CSentinel
> end() const
> requires (input_range<const _Vp> && !forward_range<const _Vp>
> && indirect_unary_predicate<const _Pred, iterator_t<const _Vp>>)
> - { return _Sentinel<true>{this}; }
> + { return _CSentinel{this}; }
> };
>
> template<typename _Range, typename _Pred>
>
More information about the Libstdc++
mailing list