Here is a new version working also in C++98. 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. 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 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 >>>> 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(__n), _M_get_Tp_allocator())); >>>> - this->_M_impl._M_end_of_storage = >>>> - this->_M_impl._M_start + static_cast(__n); >>>> - _M_fill_initialize(static_cast(__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(__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 >>> void >>> _M_initialize_dispatch(_Integer __ni, _Integer __value, __true_type) >>> { >>> const size_type __n = static_cast(__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! >