[Bug libstdc++/86127] STL containers do not satisfy container.requirements.general clause 8
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86127 Jonathan Wakely changed: What|Removed |Added Target Milestone|9.0 |7.4 --- Comment #13 from Jonathan Wakely --- (In reply to Jonathan Wakely from comment #6) > I've removed the converting copies from forward_list. For 7.4 and 8.2 as well as trunk.
[Bug libstdc++/86127] STL containers do not satisfy container.requirements.general clause 8
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86127 --- Comment #12 from Jonathan Wakely --- Author: redi Date: Wed Jul 4 13:59:42 2018 New Revision: 262411 URL: https://gcc.gnu.org/viewcvs?rev=262411&root=gcc&view=rev Log: PR libstdc++/86127 avoid unnecessary allocator conversions There is no need to use an allocator of the correct value_type when calling allocator_traits::construct and allocator_traits::destroy. The existing node allocator can be used, instead of constructing a new allocator object every time. There's also no benefit to using __gnu_cxx::__alloc_traits instead of std::allocator_traits to get the pointer and const_pointer types. std::forward_list is only available for C++11 and later, when std::allocator_traits is available too. Backport from mainline 2018-06-13 Jonathan Wakely PR libstdc++/86127 * include/bits/forward_list.h (_Fwd_list_base::_Tp_alloc_type): Remove unused typedef. (_Fwd_list_base::_M_create_node, _Fwd_list_base::_M_erase_after): Use node allocator to create and destroy elements. (forward_list::_Tp_alloc_type): Remove unused typedef. (forward_list::_Alloc_traits): Use allocator_traits instead of __gnu_cxx::__alloc_traits. * include/bits/forward_list.tcc (_Fwd_list_base::_M_erase_after): Use node allocator to create and destroy elements. Modified: branches/gcc-7-branch/libstdc++-v3/ChangeLog branches/gcc-7-branch/libstdc++-v3/include/bits/forward_list.h branches/gcc-7-branch/libstdc++-v3/include/bits/forward_list.tcc
[Bug libstdc++/86127] STL containers do not satisfy container.requirements.general clause 8
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86127 --- Comment #11 from Jonathan Wakely --- Author: redi Date: Wed Jul 4 11:46:43 2018 New Revision: 262393 URL: https://gcc.gnu.org/viewcvs?rev=262393&root=gcc&view=rev Log: PR libstdc++/86127 avoid unnecessary allocator conversions There is no need to use an allocator of the correct value_type when calling allocator_traits::construct and allocator_traits::destroy. The existing node allocator can be used, instead of constructing a new allocator object every time. There's also no benefit to using __gnu_cxx::__alloc_traits instead of std::allocator_traits to get the pointer and const_pointer types. std::forward_list is only available for C++11 and later, when std::allocator_traits is available too. Backport from mainline 2018-06-13 Jonathan Wakely PR libstdc++/86127 * include/bits/forward_list.h (_Fwd_list_base::_Tp_alloc_type): Remove unused typedef. (_Fwd_list_base::_M_create_node, _Fwd_list_base::_M_erase_after): Use node allocator to create and destroy elements. (forward_list::_Tp_alloc_type): Remove unused typedef. (forward_list::_Alloc_traits): Use allocator_traits instead of __gnu_cxx::__alloc_traits. Modified: branches/gcc-8-branch/libstdc++-v3/ChangeLog branches/gcc-8-branch/libstdc++-v3/include/bits/forward_list.h branches/gcc-8-branch/libstdc++-v3/include/bits/forward_list.tcc
[Bug libstdc++/86127] STL containers do not satisfy container.requirements.general clause 8
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86127 --- Comment #10 from Jonathan Wakely --- As I said in comment 6, I've already removed the copies that forward_list does on destruction. As I said in comment 3, there are no copies in the default constructor, they're in the initializer-list constructors. The standard says the initializer-list constructors look like: vector(initializer_list, const Allocator& = Allocator()); That default argument needs to get copied into the vector's allocator member somehow. It could be avoided by defining: vector(initializer_list); vector(initializer_list, const Allocator&); But doing so just to avoid making copies of a cheap-to-copy object is not a good justification, and would make it impossible to explicitly instantiate the container with a non-default constructible allocator type. Node-based containers such as forward_list and map neede to convert the allocator from the value_type to the node type. That can either happen once on construction, or every time elements are inserted or erased. Doing it on construction is the obvious choice, and that's where you see the "template copy constructor" lines for forward_list and map. The standard allows those copies, and they should be cheap. There is no bug here.
[Bug libstdc++/86127] STL containers do not satisfy container.requirements.general clause 8
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86127 --- Comment #9 from Scott Constable --- (In reply to Jonathan Wakely from comment #8) > (In reply to Scott Constable from comment #7) > > (In reply to Jonathan Wakely from comment #1) > > > The allocator requirements say that move construction must be equivalent > > > to > > > copy construction, and allocators should be cheap to copy anyway. I don't > > > consider this a bug. > > > > To be nitpicky, it looks like this equivalence requirement was introduced > > recently in the C++20 draft. I'm compiling using C++14. > > It's a defect report, so applies to previous standards. > > https://wg21.link/lwg2593 > > Applying that change selectively would be madness. Ah, I wasn't aware this was a defect correction. I should have figured that out myself. > > > I agree that allocators should be cheap to copy, but as a general principle > > I think that all objects should be copied only when necessary. This is the > > behavior I have observed in STL containers in libc++, as shown in my example > > above. It just makes sense to me that when an STL container is moved, its > > allocator should be moved, and no copying should be performed. > > And that's exactly what libstdc++ does. Your test fails to distinguish > between copies/moves performed during move construction and other operations. I misstated my argument here. The STL container move invokes one move from the allocator, and this is correct. What I don't understand is why the other copies, e.g. from the container default constructor and destructor, are necessary. Particularly when libc++ does not exhibit this behavior.
[Bug libstdc++/86127] STL containers do not satisfy container.requirements.general clause 8
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86127 --- Comment #8 from Jonathan Wakely --- (In reply to Scott Constable from comment #7) > (In reply to Jonathan Wakely from comment #1) > > The allocator requirements say that move construction must be equivalent to > > copy construction, and allocators should be cheap to copy anyway. I don't > > consider this a bug. > > To be nitpicky, it looks like this equivalence requirement was introduced > recently in the C++20 draft. I'm compiling using C++14. It's a defect report, so applies to previous standards. https://wg21.link/lwg2593 Applying that change selectively would be madness. > I agree that allocators should be cheap to copy, but as a general principle > I think that all objects should be copied only when necessary. This is the > behavior I have observed in STL containers in libc++, as shown in my example > above. It just makes sense to me that when an STL container is moved, its > allocator should be moved, and no copying should be performed. And that's exactly what libstdc++ does. Your test fails to distinguish between copies/moves performed during move construction and other operations.
[Bug libstdc++/86127] STL containers do not satisfy container.requirements.general clause 8
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86127 --- Comment #7 from Scott Constable --- (In reply to Jonathan Wakely from comment #1) > The allocator requirements say that move construction must be equivalent to > copy construction, and allocators should be cheap to copy anyway. I don't > consider this a bug. To be nitpicky, it looks like this equivalence requirement was introduced recently in the C++20 draft. I'm compiling using C++14. I agree that allocators should be cheap to copy, but as a general principle I think that all objects should be copied only when necessary. This is the behavior I have observed in STL containers in libc++, as shown in my example above. It just makes sense to me that when an STL container is moved, its allocator should be moved, and no copying should be performed.
[Bug libstdc++/86127] STL containers do not satisfy container.requirements.general clause 8
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86127 Jonathan Wakely changed: What|Removed |Added Status|REOPENED|RESOLVED Resolution|--- |FIXED Target Milestone|--- |9.0 Severity|normal |enhancement --- Comment #6 from Jonathan Wakely --- I've removed the converting copies from forward_list.
[Bug libstdc++/86127] STL containers do not satisfy container.requirements.general clause 8
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86127 --- Comment #5 from Jonathan Wakely --- Author: redi Date: Wed Jun 13 15:14:48 2018 New Revision: 261554 URL: https://gcc.gnu.org/viewcvs?rev=261554&root=gcc&view=rev Log: PR libstdc++/86127 avoid unnecessary allocator conversions There is no need to use an allocator of the correct value_type when calling allocator_traits::construct and allocator_traits::destroy. The existing node allocator can be used, instead of constructing a new allocator object every time. There's also no benefit to using __gnu_cxx::__alloc_traits instead of std::allocator_traits to get the pointer and const_pointer types. std::forward_list is only available for C++11 and later, when std::allocator_traits is available too. PR libstdc++/86127 * include/bits/forward_list.h (_Fwd_list_base::_Tp_alloc_type): Remove unused typedef. (_Fwd_list_base::_Node_alloc_traits): Use allocator_traits instead of __gnu_cxx::__alloc_traits. (_Fwd_list_base::_M_create_node, _Fwd_list_base::_M_erase_after): Use node allocator to create and destroy elements. (forward_list::_Tp_alloc_type): Remove unused typedef. (forward_list::_Alloc_traits): Use allocator_traits instead of __gnu_cxx::__alloc_traits. Modified: trunk/libstdc++-v3/ChangeLog trunk/libstdc++-v3/include/bits/forward_list.h trunk/libstdc++-v3/include/bits/forward_list.tcc
[Bug libstdc++/86127] STL containers do not satisfy container.requirements.general clause 8
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86127 Jonathan Wakely changed: What|Removed |Added Status|RESOLVED|REOPENED Last reconfirmed||2018-06-13 Resolution|INVALID |--- Ever confirmed|0 |1 --- Comment #4 from Jonathan Wakely --- We can avoid the "template copy constructor" lines with a trivial fix to forward_list though, and then we get: forward_list test == template copy constructor move constructor move constructor map test == copy constructor template copy constructor move constructor move constructor vector test == copy constructor move constructor All copies come from the initializer-list constructors.
[Bug libstdc++/86127] STL containers do not satisfy container.requirements.general clause 8
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86127 --- Comment #3 from Jonathan Wakely --- (In reply to Jonathan Wakely from comment #2) > Also your test is flawed. > > (In reply to Scott Constable from comment #0) > > forward_list test > > == > > These all come from the default constructor: Oops, I meant from the initializer-list constructor, not the default constructor.
[Bug libstdc++/86127] STL containers do not satisfy container.requirements.general clause 8
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86127 Jonathan Wakely changed: What|Removed |Added Status|UNCONFIRMED |RESOLVED Resolution|--- |INVALID --- Comment #2 from Jonathan Wakely --- Also your test is flawed. (In reply to Scott Constable from comment #0) > forward_list test > == These all come from the default constructor: > template copy constructor > move constructor > template copy constructor > template copy constructor > template copy constructor > template copy constructor Only this comes from the move constructor: > move constructor > map test > == These all come from the default constructor: > copy constructor > template copy constructor > move constructor Only this comes from the move constructor: > move constructor > > vector test > == Only this comes from the move constructor: > move constructor These all come from the destructors: > template copy constructor > template copy constructor > template copy constructor > template copy constructor So the move constructors all perform exactly one move construction, as required.
[Bug libstdc++/86127] STL containers do not satisfy container.requirements.general clause 8
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86127 --- Comment #1 from Jonathan Wakely --- The allocator requirements say that move construction must be equivalent to copy construction, and allocators should be cheap to copy anyway. I don't consider this a bug.
