On Fri, May 9, 2025 at 5:25 PM Jonathan Wakely <jwak...@redhat.com> wrote:
> On Fri, 9 May 2025 at 16:13, Tomasz Kaminski <tkami...@redhat.com> wrote:
> >
> >
> >
> > On Thu, May 8, 2025 at 7:46 PM Jonathan Wakely <jwak...@redhat.com>
> wrote:
> >>
> >> On Fri, 18 Apr 2025 at 10:03, Tomasz Kamiński <tkami...@redhat.com>
> wrote:
> >> >
> >> > This patch adds a _Guard_nodes scope guard nested to the _Deque_base,
> >> > that deallocates the range of nodes, and replaces __try/__catch block
> >> > with approparietly constructed guard object.
> >>
> >> "appropriately"
> >>
> >> >
> >> > libstdc++-v3/ChangeLog:
> >> >
> >> > * include/bits/deque.tcc (_Deque_base<_Tp,
> _Alloc>::_Guard_nodes): Define.
> >>
> >> There's no need for the template argument list here, just
> >> "_Deque_base" is unambiguous (there's no partial or explicit
> >> specialization that could be disambiguated with template argument
> >> lists). And just "deque" below.
> >>
> >> > (_Deque_base<_Tp, _Alloc>::_M_create_nodes): Moved defintion
> from stl_deque.h
> >> > and replace __try/__catch with _Guard_nodes scope object.
> >> > (deque<_Tp, _Alloc>::_M_fill_insert, deque<_Tp,
> _Alloc>::_M_default_append)
> >> > (deque<_Tp, _Alloc>::_M_push_back_aux, deque<_Tp,
> _Alloc>::_M_push_front_aux)
> >> > (deque<_Tp, _Alloc>::_M_range_prepend, deque<_Tp,
> _Alloc>::_M_range_append)
> >> > (deque<_Tp, _Alloc>::_M_insert_aux): Replace __try/__catch
> with _Guard_nodes
> >> > scope object.
> >> > (deque<_Tp, _Alloc>::_M_new_elements_at_back)
> >> > (deque<_Tp, _Alloc>::_M_new_elements_at_back): Use
> _M_create_nodes.
> >> > * include/bits/stl_deque.h (_Deque_base<_Tp,
> _Alloc>::_Guard_nodes): Declare.
> >> > (_Deque_base<_Tp, _Alloc)::_M_create_nodes): Move defintion
> to deque.tcc.
> >> > (deque<_Tp, _Alloc>::_Guard_nodes): Add typedef, so name is
> found by lookup.
> >> > ---
> >> > Testing x86_64-linux, default test configuration passed.
> >> > OK for trunk?
> >> >
> >> > libstdc++-v3/include/bits/deque.tcc | 424
> ++++++++++++--------------
> >> > libstdc++-v3/include/bits/stl_deque.h | 20 +-
> >> > 2 files changed, 196 insertions(+), 248 deletions(-)
> >> >
> >> > diff --git a/libstdc++-v3/include/bits/deque.tcc
> b/libstdc++-v3/include/bits/deque.tcc
> >> > index dabb6ec5365..b70eed69294 100644
> >> > --- a/libstdc++-v3/include/bits/deque.tcc
> >> > +++ b/libstdc++-v3/include/bits/deque.tcc
> >> > @@ -63,6 +63,40 @@ namespace std _GLIBCXX_VISIBILITY(default)
> >> > _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >> > _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
> >> >
> >> > + template<typename _Tp, typename _Alloc>
> >> > + struct
> >>
> >> No new line here, just "struct _Deque_base...".
> >>
> >> > + _Deque_base<_Tp, _Alloc>::_Guard_nodes
> >> > + {
> >> > + _Guard_nodes(_Deque_base& __self,
> >> > + _Map_pointer __first, _Map_pointer __last)
> >> > + : _M_self(__self), _M_first(__first), _M_last(__last)
> >> > + { }
> >> > +
> >> > + ~_Guard_nodes()
> >> > + { _M_self._M_destroy_nodes(_M_first, _M_last); }
> >> > +
> >> > + void _M_disarm()
> >> > + { _M_first = _M_last; }
> >> > +
> >> > + _Deque_base& _M_self;
> >> > + _Map_pointer _M_first;
> >> > + _Map_pointer _M_last;
> >> > +
> >> > + private:
> >> > + _Guard_nodes(_Guard_nodes const&);
> >> > + };
> >> > +
> >> > + template<typename _Tp, typename _Alloc>
> >> > + void
> >> > + _Deque_base<_Tp, _Alloc>::
> >> > + _M_create_nodes(_Map_pointer __nstart, _Map_pointer __nfinish)
> >> > + {
> >> > + _Guard_nodes __guard(*this, __nstart, __nstart);
> >> > + for (_Map_pointer& __cur = __guard._M_last; __cur < __nfinish;
> ++__cur)
> >> > + *__cur = this->_M_allocate_node();
> >> > + __guard._M_disarm();
> >> > + }
> >> > +
> >> > #if __cplusplus >= 201103L
> >> > template <typename _Tp, typename _Alloc>
> >> > void
> >> > @@ -310,35 +344,25 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
> >> > if (__pos._M_cur == this->_M_impl._M_start._M_cur)
> >> > {
> >> > iterator __new_start = _M_reserve_elements_at_front(__n);
> >> > - __try
> >> > - {
> >> > - std::__uninitialized_fill_a(__new_start,
> this->_M_impl._M_start,
> >> > - __x, _M_get_Tp_allocator());
> >> > - this->_M_impl._M_start = __new_start;
> >> > - }
> >> > - __catch(...)
> >> > - {
> >> > - _M_destroy_nodes(__new_start._M_node,
> >> > - this->_M_impl._M_start._M_node);
> >> > - __throw_exception_again;
> >> > - }
> >> > + _Guard_nodes __guard(*this, __new_start._M_node,
> >> > + this->_M_impl._M_start._M_node);
> >> > +
> >> > + std::__uninitialized_fill_a(__new_start,
> this->_M_impl._M_start,
> >> > + __x, _M_get_Tp_allocator());
> >> > + __guard._M_disarm();
> >> > + this->_M_impl._M_start = __new_start;
> >> > }
> >> > else if (__pos._M_cur == this->_M_impl._M_finish._M_cur)
> >> > {
> >> > iterator __new_finish = _M_reserve_elements_at_back(__n);
> >> > - __try
> >> > - {
> >> > - std::__uninitialized_fill_a(this->_M_impl._M_finish,
> >> > - __new_finish, __x,
> >> > - _M_get_Tp_allocator());
> >> > - this->_M_impl._M_finish = __new_finish;
> >> > - }
> >> > - __catch(...)
> >> > - {
> >> > - _M_destroy_nodes(this->_M_impl._M_finish._M_node + 1,
> >> > - __new_finish._M_node + 1);
> >> > - __throw_exception_again;
> >> > - }
> >> > + _Guard_nodes __guard(*this, this->_M_impl._M_finish._M_node
> + 1,
> >> > + __new_finish._M_node + 1);
> >> > +
> >> > + std::__uninitialized_fill_a(this->_M_impl._M_finish,
> >> > + __new_finish, __x,
> >> > + _M_get_Tp_allocator());
> >> > + __guard._M_disarm();
> >> > + this->_M_impl._M_finish = __new_finish;
> >> > }
> >> > else
> >> > _M_insert_aux(__pos, __n, __x);
> >> > @@ -350,23 +374,18 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
> >> > deque<_Tp, _Alloc>::
> >> > _M_default_append(size_type __n)
> >> > {
> >> > - if (__n)
> >> > - {
> >> > - iterator __new_finish = _M_reserve_elements_at_back(__n);
> >> > - __try
> >> > - {
> >> > - std::__uninitialized_default_a(this->_M_impl._M_finish,
> >> > - __new_finish,
> >> > - _M_get_Tp_allocator());
> >> > - this->_M_impl._M_finish = __new_finish;
> >> > - }
> >> > - __catch(...)
> >> > - {
> >> > - _M_destroy_nodes(this->_M_impl._M_finish._M_node + 1,
> >> > - __new_finish._M_node + 1);
> >> > - __throw_exception_again;
> >> > - }
> >> > - }
> >> > + if (!__n)
> >> > + return;
> >> > +
> >> > + iterator __new_finish = _M_reserve_elements_at_back(__n);
> >> > + _Guard_nodes __guard(*this, this->_M_impl._M_finish._M_node +
> 1,
> >> > + __new_finish._M_node + 1);
> >> > +
> >> > + std::__uninitialized_default_a(this->_M_impl._M_finish,
> >> > + __new_finish,
> >> > + _M_get_Tp_allocator());
> >> > + __guard._M_disarm();
> >> > + this->_M_impl._M_finish = __new_finish;
> >> > }
> >> >
> >> > template <typename _Tp, typename _Alloc>
> >> > @@ -495,24 +514,18 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
> >> >
> >> > _M_reserve_map_at_back();
> >> > *(this->_M_impl._M_finish._M_node + 1) =
> this->_M_allocate_node();
> >> > - __try
> >> > - {
> >> > + _Guard_nodes __guard(*this, this->_M_impl._M_finish._M_node,
> >> > + this->_M_impl._M_finish._M_node +
> 1);
> >> > #if __cplusplus >= 201103L
> >> > - _Alloc_traits::construct(this->_M_impl,
> >> > - this->_M_impl._M_finish._M_cur,
> >> > - std::forward<_Args>(__args)...);
> >> > + _Alloc_traits::construct(this->_M_impl,
> >> > + this->_M_impl._M_finish._M_cur,
> >> > + std::forward<_Args>(__args)...);
> >> > #else
> >> > - this->_M_impl.construct(this->_M_impl._M_finish._M_cur,
> __t);
> >> > + this->_M_impl.construct(this->_M_impl._M_finish._M_cur, __t);
> >> > #endif
> >> > -
> this->_M_impl._M_finish._M_set_node(this->_M_impl._M_finish._M_node
> >> > - + 1);
> >> > - this->_M_impl._M_finish._M_cur =
> this->_M_impl._M_finish._M_first;
> >> > - }
> >> > - __catch(...)
> >> > - {
> >> > - _M_deallocate_node(*(this->_M_impl._M_finish._M_node +
> 1));
> >>
> >> This deallocates _M_node+1 but the guard object guards
> >> [_M_node,_M_node+1). I think there's an off-by-one error here.
> >
> > Yes, indeed. I will also add a test that will detect this.
>
> Thanks. I think the rest of it looked OK, but I'll go over it again
> carefully.
>
> I did wonder about the "early" calls to _M_disarm(), do all the
> operations after the _M_disarm() calls guarantee not to throw?
>
> They are not early, they are placed before operation that "disarmed" the
deallocate in __catch. For example:
+ __guard._M_disarm();
+ this->_M_impl._M_finish = __new_finish;
+ _GLIBCXX_MOVE_BACKWARD3(__pos, __finish_n, __old_finish);
+ std::copy(__first, __last, __pos);
}
- __catch(...)
+ else
{
// Setting this->_M_impl._M_finish to __new_finish made this range empty,
and disarmed.
- _M_destroy_nodes(this->_M_impl._M_finish._M_node + 1,
- __new_finish._M_node + 1);
