On 27/04/2014 15:39, Jonathan Wakely wrote: > On 17/04/14 22:43 +0200, François Dumont wrote: >> Hi >> >> Here is a patch to globally enhance debug containers implementation. > > François, sorry for the delay, this is a large patch and I wanted to > give it the time it deserves to review properly. No problem, I see that there are a lot of proposals lately. > > I understand why this is needed, but it changes the layout of the > classes in very significant ways, meaning the debug containers will > not be compatible across GCC releases. I'm OK with that now, but from > the next major GCC release I'd like to avoid that in future. I remember Paolo saying that there is no abi guaranty for debug mode this is why I didn't hesitate in making this proposal. Will there be one in the future ? I plan also breaking changes for profile mode to fix its very bad performance. > >> I noticed also that in std/c++11/debug.cc we have some methods >> qualified with noexcept while in a C++03 user code those methods will >> have a throw() qualification. Is that fine ? > > As I said in my last mail, yes, those specifications are compatible. > But I don't think your changes are doing what you think they are doing > in all cases. Using _GLIBCXX_NOEXCEPT does not expand to throw() in > C++03 mode, it expands to nothing. Yes, I discover this difference in one of your recent mail. > >> * include/debug/safe_unordered_container.tcc > > N.B. This file has no changes listed in the changelog entry. I reviewed the ChangeLog and limit modifications like in this file. Note however that patch have been generated with '-x -b' option to hide white space modifications. I clean usage of white chars in impacted files, replaced some white spaces with tabs and remove useless white spaces. > >> @@ -69,8 +75,26 @@ >> >> // 23.2.1.1 construct/copy/destroy: >> >> - deque() : _Base() { } >> +#if __cplusplus < 201103L >> + deque() >> + : _Base() { } >> >> + deque(const deque& __x) >> + : _Base(__x) { } >> + >> + ~deque() _GLIBCXX_NOEXCEPT { } > > In C++03 mode the _GLIBCXX_NOEXCEPT macro expands to an empty string, > so it is useless in this chunk of code, which is only compiled for > C++03 mode. It should probably just be removed here (and in all the > other debug containers which use it in C++03-only code). Ok, I cleaned those. Did you mean removing the whole explicit destructor ? Is it a coding Standard to always explicitly implement the destructor or just a way to have Doxygen generate ? > + * before-begin ownership.*/ >> + template >> + void >> + _Safe_forward_list<_SafeSequence>:: >> + _M_swap(_Safe_sequence_base& __other) noexcept >> + { >> + __gnu_cxx::__scoped_lock sentry(_M_this()._M_get_mutex()); > > Shouldn't we be locking both containers' mutexes here? > As we do in src/c++11/debug.cc Good point, not a regression but nice to fix in this patch. > >> + forward_list(forward_list&& __list, const allocator_type& __al) >> + : _Safe(std::move(__list), __al), >> + _Base(std::move(__list), __al) >> + { } > > This makes me feel uneasy, seeing a moved-from object being used > again, but I don't think changing it to use static_casts to the two > base classes would look better, so let's leave it like that. That indeed looks scary, we replaced with: forward_list(forward_list&& __list, const allocator_type& __al) : _Safe(std::move(__list._M_safe()), __al), _Base(std::move(__list._M_base()), __al) { } it makes clearer the fact that we move each part. > >> Index: include/debug/safe_base.h >> =================================================================== >> --- include/debug/safe_base.h (revision 209446) >> +++ include/debug/safe_base.h (working copy) >> @@ -188,22 +188,18 @@ >> >> protected: >> // Initialize with a version number of 1 and no iterators >> - _Safe_sequence_base() >> + _Safe_sequence_base() _GLIBCXX_NOEXCEPT > > This use of _GLIBCXX_NOEXCEPT are correct, if the intention is to be > noexcept in C++11 and have no exception specification in C++98/C++03. Yes, I preferred to use default implementation for special function in C++11 so I qualified as many things as possible noexcept so that resulting noexcept qualification depends only on the normal mode noexcept qualification. > >> : _M_iterators(0), _M_const_iterators(0), _M_version(1) >> { } >> >> #if __cplusplus >= 201103L >> _Safe_sequence_base(const _Safe_sequence_base&) noexcept >> : _Safe_sequence_base() { } >> - >> - _Safe_sequence_base(_Safe_sequence_base&& __x) noexcept >> - : _Safe_sequence_base() >> - { _M_swap(__x); } >> #endif >> >> /** Notify all iterators that reference this sequence that the >> sequence is being destroyed. */ >> - ~_Safe_sequence_base() >> + ~_Safe_sequence_base() _GLIBCXX_NOEXCEPT > > This is redundant. In C++03 the macro expands to nothing, and in C++11 > destructors are noexcept by default anyway, so the macro adds nothing. Ok, I didn't knew, I cleaned those then. > >> { this->_M_detach_all(); } >> >> /** Detach all iterators, leaving them singular. */ >> @@ -231,7 +227,7 @@ >> * one container now reference the other container. >> */ >> void >> - _M_swap(_Safe_sequence_base& __x); >> + _M_swap(_Safe_sequence_base& __x) _GLIBCXX_NOEXCEPT; > > This is OK, if the intention is to only have an exc eption spec in > C++11. This is used in noexcept constructors so I prefer to make it noexcept too. I replaced this one with _GLIBCXX_USE_NOEXCEPT cause in src/c++11/debug.cc we always build it as noexcept, just in case it impacts the mangled name. I saw _GLIBCXX_NOTHROW in c++config but doesn't seem to be used. > > >> +#if __cplusplus >= 201103L >> + _Safe_container& >> + operator=(_Safe_container&& __x) noexcept >> + { >> + __glibcxx_check_self_move_assign(__x); > > N.B. I'm writing a paper for the next committee meeting to make it > clear that self-move-assignment is not undefined behaviour, so this > check will have to be removed in the future. At least your change > means there are fewer places to remove it from :-) > See http://gcc.gnu.org/PR59603 for some background. > > I only looked over the testsuite changes quickly, but they look good. > > The changes above are only minor, overall I like the proposal very > much. Thanks for working on this, one day our debug mode containers > will be perfect! :-) > Not yet, I will start submitting patches for the debug algos soon :-) Here is the patch again with all your remarks. Ok to commit with the following ChangeLog ? 2014-04-29 François Dumont * include/debug/macros.h [__glibcxx_check_equal_allocs]: Add parameter to pass the 2 instances to check allocator equality. * include/debug/safe_container.h: New, define _Safe_container<>. * include/Makefile.am: Add previous. * include/debug/deque (std::__debug::deque<>): Inherit _Safe_container<>. Use default implementation for all special functions. * include/debug/forward_list (std::__debug::forward_list<>): Likewise. * include/debug/list (std::__debug::list<>): Likewise. * include/debug/map.h (std::__debug::map<>): Likewise. * include/debug/multimap.h (std::__debug::multimap<>): Likewise. * include/debug/set.h (std::__debug::set<>): Likewise. * include/debug/multiset.h (std::__debug::multiset<>): Likewise. * include/debug/string (std::__debug::basic_string<>): Likewise. * include/debug/unordered_map (std::__debug::unordered_map<>): Likewise. (std::__debug::unordered_multimap<>): Likewise. * include/debug/unordered_set (std::__debug::unordered_set<>): Likewise. (std::__debug::unordered_multiset<>): Likewise. * include/debug/vector (std::__debug::vector<>): Likewise. * include/debug/safe_base.h (_Safe_sequence_base()): Add noexcept. (_Safe_sequence_base(_Safe_sequence_base&&): Remove. (~_Safe_sequence_base()): Add noexcept. * include/debug/safe_sequence.h (std::__debug::_Safe_node_sequence<>): New. * include/debug/safe_unordered_base.h (_Safe_unordered_container_base()): Add noexcept. (~_Safe_unordered_container_base()): Likewise. (_M_swap(_Safe_unordered_container_base&)): Likewise. * include/debug/safe_unordered_container.h: (_Safe_unordered_container<>::_M_invalidate_locals()): New. (_Safe_unordered_container<>::_M_invalidate_all()): New. * src/c++11/debug.cc: Limit includes, adapt methods noexcept qualifications. * testsuite/util/debug/checks.h (check_construct1): Just implement an invalid constructor invocation and no other operations potentially not supported by some types of container. (check_construct2): Likewise. (check_construct3): Likewise. * testsuite/23_containers/forward_list/allocator/move.cc: Add check on iterators to make sure they are correctly moved in debug mode. * testsuite/23_containers/forward_list/allocator/move_assign.cc: Likewise. * testsuite/23_containers/map/allocator/move.cc: Likewise. * testsuite/23_containers/map/allocator/move_assign.cc: Likewise. * testsuite/23_containers/multimap/allocator/move.cc: Likewise. * testsuite/23_containers/multimap/allocator/move_assign.cc: Likewise. * testsuite/23_containers/multiset/allocator/move.cc: Likewise. * testsuite/23_containers/multiset/allocator/move_assign.cc: Likewise. * testsuite/23_containers/set/allocator/move.cc: Likewise. * testsuite/23_containers/set/allocator/move_assign.cc: Likewise. * testsuite/23_containers/unordered_map/allocator/move.cc: Likewise. * testsuite/23_containers/unordered_map/allocator/move_assign.cc: Likewise. * testsuite/23_containers/unordered_multimap/allocator/move.cc: Likewise. * testsuite/23_containers/unordered_multimap/allocator/move_assign.cc: Likewise. * testsuite/23_containers/unordered_multiset/allocator/move.cc: Likewise. * testsuite/23_containers/unordered_multiset/allocator/move_assign.cc: Likewise. * testsuite/23_containers/unordered_set/allocator/move.cc: Likewise. * testsuite/23_containers/unordered_set/allocator/move_assign.cc: Likewise. * testsuite/23_containers/forward_list/debug/construct1_neg.cc: New. * testsuite/23_containers/forward_list/debug/construct2_neg.cc: New. * testsuite/23_containers/forward_list/debug/construct3_neg.cc: New. * testsuite/23_containers/forward_list/debug/construct4_neg.cc: New. * testsuite/23_containers/forward_list/debug/move_assign_neg.cc: New. * testsuite/23_containers/forward_list/debug/move_neg.cc: New. * testsuite/23_containers/map/debug/construct5_neg.cc: New. * testsuite/23_containers/map/debug/move_assign_neg.cc: New. * testsuite/23_containers/map/debug/move_neg.cc: New. * testsuite/23_containers/multimap/debug/construct5_neg.cc: New. * testsuite/23_containers/multimap/debug/move_assign_neg.cc: New. * testsuite/23_containers/multimap/debug/move_neg.cc: New. * testsuite/23_containers/multiset/debug/construct5_neg.cc: New. * testsuite/23_containers/multiset/debug/move_assign_neg.cc: New. * testsuite/23_containers/multiset/debug/move_neg.cc: New. * testsuite/23_containers/set/debug/construct5_neg.cc: New. * testsuite/23_containers/set/debug/move_assign_neg.cc: New. * testsuite/23_containers/set/debug/move_neg.cc: New. * testsuite/23_containers/unordered_map/debug/construct5_neg.cc: New. * testsuite/23_containers/unordered_map/debug/move_assign_neg.cc: New. * testsuite/23_containers/unordered_map/debug/move_neg.cc: New. * testsuite/23_containers/unordered_multimap/debug/construct5_neg.cc: New. * testsuite/23_containers/unordered_multimap/debug/move_assign_neg.cc: New. * testsuite/23_containers/unordered_multimap/debug/move_neg.cc: New. * testsuite/23_containers/unordered_multiset/debug/construct5_neg.cc: New. * testsuite/23_containers/unordered_multiset/debug/move_assign_neg.cc: New. * testsuite/23_containers/unordered_multiset/debug/move_neg.cc: New. * testsuite/23_containers/unordered_set/debug/construct5_neg.cc: New. * testsuite/23_containers/unordered_set/debug/move_assign_neg.cc: New. * testsuite/23_containers/unordered_set/debug/move_neg.cc: New. * testsuite/23_containers/vector/debug/move_neg.cc: New. François