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<typename _SafeSequence>
+ 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 <fdumont@gcc.gnu.org>
* 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