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?
