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 ? François On 28/05/2024 12:30, Jonathan Wakely wrote: > On Mon, 27 May 2024 at 05:37, François Dumont 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 > + (__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 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! >>>