On Wed, 25 Jan 2023 at 18:38, François Dumont <frs.dum...@gmail.com> wrote: > > Let's submit a proper patch proposal then. > > The occasion for me to ask if there is any reason for cow string not > being C++11 allocator compliant ? Just lack of interest ?
Mostly lack of interest, but also I don't really want to "encourage" the use of the old string by investing lots of maintenance effort into it. If you want new features like C++11 Allocators and resize_and_overwrite etc then you should use the new type. I don't remember if there were any actual blockers that made it difficult to support stateful allocators in the COW string. I might have written something about it in mails to the list when I was adding the SSO string, but I don't remember now. Anyway, for this patch ... > > I wanted to consider it to get rid of the __gnu_debug::_Safe_container > _IsCxx11AllocatorAware template parameter. > > libstdc++: Optimize basic_string move assignment > > Since resolution of Issue 2593 [1] we can consider that equal > allocators > before the propagate-on-move-assignment operations will still be equal > afterward. > > So we can extend the optimization of transfering the storage of the > move-to > instance to the move-from one that is currently limited to always equal > allocators. > > [1] https://cplusplus.github.io/LWG/issue2593 > > libstdc++-v3/ChangeLog: > > * include/bits/basic_string.h (operator=(basic_string&&)): > Transfer move-to > storage to the move-from instance when allocators are equal. > * > testsuite/21_strings/basic_string/allocator/char/move_assign.cc (test04): > New test case. > > Tested under linux x86_64, ok to commit ? OK for trunk, thanks! +Reviewed-by: Jonathan Wakely <jwak...@redhat.com> > > On Wed, 4 Jan 2023 at 18:21, François Dumont via Libstdc++ > > <libstd...@gcc.gnu.org> wrote: > >> On 04/01/23 00:11, waffl3x via Libstdc++ wrote: > >>> Example: https://godbolt.org/z/sKhGqG1qK > >>> https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=libstdc%2B%2B-v3/include/bits/basic_string.h;hb=HEAD#l880 > >>> When move assigning to a basic_string, the allocated memory of the moved > >>> into string is stored into the source string instead of deallocating it, > >>> a good optimization when everything is compatible. However in the case of > >>> a stateful allocator (is_always_true() evaluating as false) this > >>> optimization is never taken. Unless there is some reason I can't think of > >>> that makes equal stateful allocators incompatible here, I believe the if > >>> statement on line 880 of basic_string.h should also compare the equality > >>> of each strings allocator. The first condition in the function seems to > >>> indicate to me that this scenario was being considered and just forgotten > >>> about, as the memory doesn't get deallocated immediately if the two > >>> allocators are equal. I'll note that because of how everything is > >>> handled, this doesn't result in a leak so this bug is still only a minor > >>> missed optimization. > >>> > >>> mailto:libstd...@gcc.gnu.org > >> Hmmm, I don't know, at least it is not as simple as you present it. > >> > >> You cannot add a check on allocator equality as you are proposing > >> because it is too late. __str allocator might have already been > >> propagated to *this on the previous call to std::__alloc_on_move. Note > >> that current check is done only if > >> !_Alloc_traits::_S_propagate_on_move_assign(). > >> > >> This patch might do the job but I wonder if equal allocators can become > >> un-equal after the propagate-on-move-assignment ? > > Since https://cplusplus.github.io/LWG/issue2593 they can't. But I > > think when I wrote that code, they could do, which is probably why the > > optimization wasn't done. > >
