[PATCH 1/2] libstdc++: Implement P3725R3 Filter View Extensions for Safer Use
Patrick Palka
ppalka@redhat.com
Wed Apr 1 17:35:14 GMT 2026
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:
-- >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