debug container patch
Jonathan Wakely
jwakely@redhat.com
Sun Apr 27 14:43:00 GMT 2014
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.
> I have isolated all code of special functions in a base class so
>that in C++11 we can use default implementations for the debug
>containers. This way implementation is simpler and inherit from the
>noexcept qualifications.
I like this approach, the result is simpler and less repetitive, but
I'm slightly concerned by inverting the inheritance ...
> _Safe_container is now using the debug base class
>(_Safe_sequence, _Safe_node_sequence, _Safe_forward_list...) as a
>policy describing what must be done as _M_invalidate_all or _M_swap.
>Note that I needed to invert inheritance so that I can use this base
>type to do checks with allocators before they get potentially moved
>by the normal code.
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.
> This is the case for the allocator aware move
>constructor and move assignment operator. With the allocator aware
>move constructor it is in fact fixing a bug, iterators were not
>correctly swap when they had to.
Great, I had been meaning to fully review the iterator handling in
those kind of cases, thanks for finding and fixing this.
> I had to put a _IsCpp11AllocatorAware template parameter to this
>new type for types that are not yet C++11 allocator aware. We will be
>able to simplify it later.
Yes, that should become unnecessary by the end of stage 1, but for now
it serves a purpose. As I said in my other email, please rename it to
_IsCxx11AllocatorAware, for consistency with the rest of the library.
> 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.
If you want a macro that expands to either throw() or noexcept, then
you should be using _GLIBCXX_USE_NOEXCEPT.
> * include/debug/safe_sequence.tcc
> * include/debug/safe_unordered_base.h
> * include/debug/safe_unordered_container.h
> (_Safe_unordered_container_base()): Add noexcept.
> (~_Safe_unordered_container_base()): Likewise.
> (_M_swap(_Safe_unordered_container_base&)): Likewise.
> * include/debug/safe_unordered_container.tcc
N.B. This file has no changes listed in the changelog entry.
>@@ -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).
>Index: include/debug/forward_list
>===================================================================
>--- include/debug/forward_list (revision 209446)
>+++ include/debug/forward_list (working copy)
>@@ -33,8 +33,113 @@
>
> #include <forward_list>
> #include <debug/safe_sequence.h>
>+#include <debug/safe_container.h>
> #include <debug/safe_iterator.h>
>
>+namespace __gnu_debug
>+{
>+ /// Special iterators swap and invalidation for forward_list because of the
>+ /// before_begin iterator.
>+ template<typename _SafeSequence>
>+ class _Safe_forward_list
>+ : public _Safe_sequence<_SafeSequence>
>+ {
>+ _SafeSequence&
>+ _M_this() noexcept
>+ { return *static_cast<_SafeSequence*>(this); }
>+
>+ static void
>+ _M_swap_aux(_Safe_sequence_base& __lhs,
>+ _Safe_iterator_base*& __lhs_iterators,
>+ _Safe_sequence_base& __rhs,
>+ _Safe_iterator_base*& __rhs_iterators);
>+
>+ protected:
>+ void
>+ _M_invalidate_all()
>+ {
>+ using _Base_const_iterator = __decltype(_M_this()._M_base().cend());
>+ this->_M_invalidate_if([this](_Base_const_iterator __it)
>+ {
>+ return __it != _M_this()._M_base().cbefore_begin()
>+ && __it != _M_this()._M_base().cend(); });
>+ }
>+
>+ void _M_swap(_Safe_sequence_base&) noexcept;
>+ };
>+
>+ template<typename _SafeSequence>
>+ void
>+ _Safe_forward_list<_SafeSequence>::
>+ _M_swap_aux(_Safe_sequence_base& __lhs,
>+ _Safe_iterator_base*& __lhs_iterators,
>+ _Safe_sequence_base& __rhs,
>+ _Safe_iterator_base*& __rhs_iterators)
>+ {
>+ using const_iterator = typename _SafeSequence::const_iterator;
>+ _Safe_iterator_base* __bbegin_its = 0;
>+ _Safe_iterator_base* __last_bbegin = 0;
>+ _SafeSequence& __rseq = static_cast<_SafeSequence&>(__rhs);
>+
>+ for (_Safe_iterator_base* __iter = __lhs_iterators; __iter;)
>+ {
>+ // Even iterator are casted to const_iterator, not a problem.
"is cast" not "are casted" please
>+ const_iterator* __victim = static_cast<const_iterator*>(__iter);
>+ __iter = __iter->_M_next;
>+ if (__victim->base() == __rseq._M_base().cbefore_begin())
>+ {
>+ __victim->_M_unlink();
>+ if (__lhs_iterators == __victim)
>+ __lhs_iterators = __victim->_M_next;
>+ if (__bbegin_its)
>+ {
>+ __victim->_M_next = __bbegin_its;
>+ __bbegin_its->_M_prior = __victim;
>+ }
>+ else
>+ __last_bbegin = __victim;
>+ __bbegin_its = __victim;
>+ }
>+ else
>+ __victim->_M_sequence = &__lhs;
>+ }
>+
>+ if (__bbegin_its)
>+ {
>+ if (__rhs_iterators)
>+ {
>+ __rhs_iterators->_M_prior = __last_bbegin;
>+ __last_bbegin->_M_next = __rhs_iterators;
>+ }
>+ __rhs_iterators = __bbegin_its;
>+ }
>+ }
>+
>+ /* Special forward_list _M_swap version that do not swap the
"does not" instead of "do not" please
>+ * 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
>+ 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.
>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.
> : _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.
> { 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.
>+#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.
> #include <debug/safe_sequence.tcc>
>Index: include/debug/safe_unordered_base.h
>===================================================================
>--- include/debug/safe_unordered_base.h (revision 209446)
>+++ include/debug/safe_unordered_base.h (working copy)
>@@ -132,15 +132,16 @@
>
> protected:
> // Initialize with a version number of 1 and no iterators
>- _Safe_unordered_container_base()
>+ _Safe_unordered_container_base() noexcept
> : _M_local_iterators(nullptr), _M_const_local_iterators(nullptr)
> { }
>
>- // Initialize with a version number of 1 and no iterators
>+ // Copy constructor do not copy iterators.
"does not" instead of "do not"
> _Safe_unordered_container_base(const _Safe_unordered_container_base&)
> noexcept
> : _Safe_unordered_container_base() { }
>
>+ // When moved unordered containers iterators are swapped.
> _Safe_unordered_container_base(_Safe_unordered_container_base&& __x)
> noexcept
> : _Safe_unordered_container_base()
>@@ -148,7 +149,7 @@
>
> /** Notify all iterators that reference this container that the
> container is being destroyed. */
>- ~_Safe_unordered_container_base()
>+ ~_Safe_unordered_container_base() noexcept
This noexcept is redundant.
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! :-)
More information about the Gcc-patches
mailing list