On Tue, 28 May 2024 at 21:55, François Dumont <frs.dum...@gmail.com> wrote: > > I can indeed restore _M_initialize_dispatch as it was before. It was not > fixing my initial problem. I simply kept the code simplification. > > libstdc++: Use RAII to replace try/catch blocks > > Move _Guard into std::vector declaration and use it to guard all > calls to > vector _M_allocate. > > Doing so the compiler has more visibility on what is done with the > pointers > and do not raise anymore the -Wfree-nonheap-object warning. > > libstdc++-v3/ChangeLog: > > * include/bits/vector.tcc (_Guard): Move all the nested > duplicated class... > * include/bits/stl_vector.h (_Guard_alloc): ...here and rename. > (_M_allocate_and_copy): Use latter. > (_M_initialize_dispatch): Small code simplification. > (_M_range_initialize): Likewise and set _M_finish first > from the result > of __uninitialize_fill_n_a that can throw. > > Tested under Linux x86_64. > > Ok to commit ?
OK, thanks > > François > > On 28/05/2024 12:30, Jonathan Wakely wrote: > > On Mon, 27 May 2024 at 05:37, François Dumont <frs.dum...@gmail.com> wrote: > >> Here is a new version working also in C++98. > > Can we use a different solution that doesn't involve an explicit > > template argument list for that __uninitialized_fill_n_a call? > > > > -+ this->_M_impl._M_finish = std::__uninitialized_fill_n_a > > ++ this->_M_impl._M_finish = > > ++ std::__uninitialized_fill_n_a<pointer, size_type, value_type> > > + (__start, __n, __value, _M_get_Tp_allocator()); > > > > Using _M_fill_initialize solves the problem :-) > > > > > > > >> Note that I have this failure: > >> > >> FAIL: 23_containers/vector/types/1.cc -std=gnu++98 (test for excess > >> errors) > >> > >> but it's already failing on master, my patch do not change anything. > > Yes, that's been failing for ages. > > > >> Tested under Linux x64, > >> > >> still ok to commit ? > >> > >> François > >> > >> On 24/05/2024 16:17, Jonathan Wakely wrote: > >>> On Thu, 23 May 2024 at 18:38, François Dumont <frs.dum...@gmail.com> > >>> wrote: > >>>> On 23/05/2024 15:31, Jonathan Wakely wrote: > >>>>> On 23/05/24 06:55 +0200, François Dumont wrote: > >>>>>> As explained in this email: > >>>>>> > >>>>>> https://gcc.gnu.org/pipermail/libstdc++/2024-April/058552.html > >>>>>> > >>>>>> I experimented -Wfree-nonheap-object because of my enhancements on > >>>>>> algos. > >>>>>> > >>>>>> So here is a patch to extend the usage of the _Guard type to other > >>>>>> parts of vector. > >>>>> Nice, that fixes the warning you were seeing? > >>>> Yes ! I indeed forgot to say so :-) > >>>> > >>>> > >>>>> We recently got a bug report about -Wfree-nonheap-object in > >>>>> std::vector, but that is coming from _M_realloc_append which already > >>>>> uses the RAII guard :-( > >>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115016 > >>>> Note that I also had to move call to __uninitialized_copy_a before > >>>> assigning this->_M_impl._M_start so get rid of the -Wfree-nonheap-object > >>>> warn. But _M_realloc_append is already doing potentially throwing > >>>> operations before assigning this->_M_impl so it must be something else. > >>>> > >>>> Though it made me notice another occurence of _Guard in this method. Now > >>>> replaced too in this new patch. > >>>> > >>>> libstdc++: Use RAII to replace try/catch blocks > >>>> > >>>> Move _Guard into std::vector declaration and use it to guard all > >>>> calls to > >>>> vector _M_allocate. > >>>> > >>>> Doing so the compiler has more visibility on what is done with the > >>>> pointers > >>>> and do not raise anymore the -Wfree-nonheap-object warning. > >>>> > >>>> libstdc++-v3/ChangeLog: > >>>> > >>>> * include/bits/vector.tcc (_Guard): Move all the nested > >>>> duplicated class... > >>>> * include/bits/stl_vector.h (_Guard_alloc): ...here. > >>>> (_M_allocate_and_copy): Use latter. > >>>> (_M_initialize_dispatch): Likewise and set _M_finish first > >>>> from the result > >>>> of __uninitialize_fill_n_a that can throw. > >>>> (_M_range_initialize): Likewise. > >>>> > >>>>>> diff --git a/libstdc++-v3/include/bits/stl_vector.h > >>>>>> b/libstdc++-v3/include/bits/stl_vector.h > >>>>>> index 31169711a48..4ea74e3339a 100644 > >>>>>> --- a/libstdc++-v3/include/bits/stl_vector.h > >>>>>> +++ b/libstdc++-v3/include/bits/stl_vector.h > >>>>>> @@ -1607,6 +1607,39 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER > >>>>>> clear() _GLIBCXX_NOEXCEPT > >>>>>> { _M_erase_at_end(this->_M_impl._M_start); } > >>>>>> > >>>>>> + private: > >>>>>> + // RAII guard for allocated storage. > >>>>>> + struct _Guard > >>>>> If it's being defined at class scope instead of locally in a member > >>>>> function, I think a better name would be good. Maybe _Ptr_guard or > >>>>> _Dealloc_guard or something. > >>>> _Guard_alloc chosen. > >>>>>> + { > >>>>>> + pointer _M_storage; // Storage to deallocate > >>>>>> + size_type _M_len; > >>>>>> + _Base& _M_vect; > >>>>>> + > >>>>>> + _GLIBCXX20_CONSTEXPR > >>>>>> + _Guard(pointer __s, size_type __l, _Base& __vect) > >>>>>> + : _M_storage(__s), _M_len(__l), _M_vect(__vect) > >>>>>> + { } > >>>>>> + > >>>>>> + _GLIBCXX20_CONSTEXPR > >>>>>> + ~_Guard() > >>>>>> + { > >>>>>> + if (_M_storage) > >>>>>> + _M_vect._M_deallocate(_M_storage, _M_len); > >>>>>> + } > >>>>>> + > >>>>>> + _GLIBCXX20_CONSTEXPR > >>>>>> + pointer > >>>>>> + _M_release() > >>>>>> + { > >>>>>> + pointer __res = _M_storage; > >>>>>> + _M_storage = 0; > >>>>> I don't think the NullablePointer requirements include assigning 0, > >>>>> only from nullptr, which isn't valid in C++98. > >>>>> > >>>>> https://en.cppreference.com/w/cpp/named_req/NullablePointer > >>>>> > >>>>> Please use _M_storage = pointer() instead. > >>>> I forgot about user fancy pointer, fixed. > >>>> > >>>> > >>>>>> + return __res; > >>>>>> + } > >>>>>> + > >>>>>> + private: > >>>>>> + _Guard(const _Guard&); > >>>>>> + }; > >>>>>> + > >>>>>> protected: > >>>>>> /** > >>>>>> * Memory expansion handler. Uses the member allocation > >>>>>> function to > >>>>>> @@ -1618,18 +1651,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER > >>>>>> _M_allocate_and_copy(size_type __n, > >>>>>> _ForwardIterator __first, _ForwardIterator __last) > >>>>>> { > >>>>>> - pointer __result = this->_M_allocate(__n); > >>>>>> - __try > >>>>>> - { > >>>>>> - std::__uninitialized_copy_a(__first, __last, __result, > >>>>>> - _M_get_Tp_allocator()); > >>>>>> - return __result; > >>>>>> - } > >>>>>> - __catch(...) > >>>>>> - { > >>>>>> - _M_deallocate(__result, __n); > >>>>>> - __throw_exception_again; > >>>>>> - } > >>>>>> + _Guard __guard(this->_M_allocate(__n), __n, *this); > >>>>>> + std::__uninitialized_copy_a > >>>>>> + (__first, __last, __guard._M_storage, _M_get_Tp_allocator()); > >>>>>> + return __guard._M_release(); > >>>>>> } > >>>>>> > >>>>>> > >>>>>> @@ -1642,13 +1667,15 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER > >>>>>> // 438. Ambiguity in the "do the right thing" clause > >>>>>> template<typename _Integer> > >>>>>> void > >>>>>> - _M_initialize_dispatch(_Integer __n, _Integer __value, > >>>>>> __true_type) > >>>>>> + _M_initialize_dispatch(_Integer __int_n, _Integer __value, > >>>>>> __true_type) > >>>>>> { > >>>>>> - this->_M_impl._M_start = _M_allocate(_S_check_init_len( > >>>>>> - static_cast<size_type>(__n), _M_get_Tp_allocator())); > >>>>>> - this->_M_impl._M_end_of_storage = > >>>>>> - this->_M_impl._M_start + static_cast<size_type>(__n); > >>>>>> - _M_fill_initialize(static_cast<size_type>(__n), __value); > >>>>> Please fix the comment on _M_fill_initialize if you're removing the > >>>>> use of it here. > >>>> Already done in this initial patch proposal, see below. > >>>> > >>>>>> + const size_type __n = static_cast<size_type>(__int_n); > >>>>>> + _Guard __guard(_M_allocate(_S_check_init_len( > >>>>>> + __n, _M_get_Tp_allocator())), __n, *this); > >>>>> I think this would be easier to read if the _S_check_init_len call was > >>>>> done first, and maybe the allocation too, since we are going to need a > >>>>> local __start later anyway. So maybe like this: > >>>>> > >>>>> template<typename _Integer> > >>>>> void > >>>>> _M_initialize_dispatch(_Integer __ni, _Integer __value, > >>>>> __true_type) > >>>>> { > >>>>> const size_type __n = static_cast<size_type>(__ni); > >>>>> pointer __start = _M_allocate(_S_check_init_len(__n), > >>>>> _M_get_Tp_allocator()); > >>>>> _Guard __guard(__start, __n, *this); > >>>>> this->_M_impl._M_start = __start; > >>>>> _M_fill_initialize(__n, __value); > >>>>> this->_M_impl._M_end_of_storage = __start + __n; > >>>>> (void) __guard._M_release(); > >>>>> } > >>>>> > >>>>> Or inline the __uninitialized_fill_n_a call if you want to (but then > >>>>> fix the comment on _M_fill_initialize). Inlining it does make this > >>>>> function more consistent with the next one, which calls > >>>>> __uninitialized_copy_a directly. > >>>> Yes, this is why I called __uninitialized_fill_n_a instead and also to > >>>> do so *before* assigning _M_impl._M_start. > >>>> > >>>> > >>>>>> - // Called by the first initialize_dispatch above and by the > >>>>>> - // vector(n,value,a) constructor. > >>>>>> + // Called by the vector(n,value,a) constructor. > >>>> See, it's here :-) > >>> Doh! Sorry, I'm not sure how I missed that. > >>> > >>>> Ok to commit ? > >>> OK for trunk, thanks! > >>>
