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!
