Re: debug container patch
On Wed, May 7, 2014 at 2:13 AM, Paolo Carlini paolo.carl...@oracle.com wrote: -- Francois, remember to regenerate and commit the Makefile.in changes. Can someone regenerate and commit the Makefile.in changes soon ? I'm seeing testsuite failures thanks to missing debug/safe_container.h on arm-none-linux-gnueabihf I don't have access to a machine right now with the right versions of autoconf and automake that can do this easily. Ramana Thanks, Paolo.
Re: debug container patch
On 07/05/14 14:17 +0100, Ramana Radhakrishnan wrote: Can someone regenerate and commit the Makefile.in changes soon ? I'm seeing testsuite failures thanks to missing debug/safe_container.h on arm-none-linux-gnueabihf It was done hours ago by http://gcc.gnu.org/ml/gcc-cvs/2014-05/msg00170.html
Re: debug container patch
On Wed, May 7, 2014 at 2:22 PM, Jonathan Wakely jwak...@redhat.com wrote: On 07/05/14 14:17 +0100, Ramana Radhakrishnan wrote: Can someone regenerate and commit the Makefile.in changes soon ? I'm seeing testsuite failures thanks to missing debug/safe_container.h on arm-none-linux-gnueabihf It was done hours ago by http://gcc.gnu.org/ml/gcc-cvs/2014-05/msg00170.html Sorry about the noise. I realized that just after I had hit send. not enough coffee today. Ramana
Re: debug container patch
On 28/04/14 23:07 +0200, François Dumont wrote:
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.
Yes, I need to review patches faster!
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 ?
Probably not. Originally the performance impact of debug mode was so
large that noone would use the containers except for special builds to
find specific problems. With some of your performance improvements
(unwrapping safe iterators once we know they are valid etc.) I wonder
if some people would choose to use e.g. __gnu_debug::vector directly
in release builds. If anyone is doing that they would probably want
a stable ABI, but we don't guarantee that now, and probably shouldn't
guarantee it in future.
If people want extra safety without _GLIBCXX_DEBUG we should probably
consider the lightweight checks that have been implemented on the
Google branch.
I plan also breaking changes for profile mode to
fix its very bad performance.
I don't care about Profile mode at all! :-)
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.
Great, thank you.
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 ?
What you've done in your patch is fine.
We don't have any such coding standard for destructors though (AFAIK!)
To avoid losing the Doxygen comments we could put them on the
defaulted C++11 destructor. If we don't do so already we should
consider generating Doxygen docs with -std=gnu++11 (it's not the
default used by G++, but it would show more complete, and more modern,
API information).
+ * before-begin ownership.*/
+ templatetypename _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.
Looks good now, thanks.
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.
Yes, this is much less scary :-)
Thanks, the revised patch is OK for trunk.
Re: debug container patch
-- Francois, remember to regenerate and commit the Makefile.in changes. Thanks, Paolo.
Re: debug container patch
Hi Jonathan
I just wanted to make sure that you are aware that I preferred to
wait for another validation of the small modification I have done.
François
On 28/04/2014 23:07, François Dumont wrote:
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.*/
+ templatetypename _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
Re: debug container patch
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.
+ templatetypename _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;
+};
+
+ templatetypename _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)
+{
+
Re: debug container patch
On 17 April 2014 21:43, François Dumont wrote: Hi Here is a patch to globally enhance debug containers implementation. 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. Excellent. 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. Minor: we switch from using cpp to cxx, meaning cpp can unambiguously refer to the preprocessor, so I would use _IsCxx11... for that. 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 ? Yes, an empty throw() is compatible with noexcept(true). I'll review the rest of the patch over the weekend, thanks!
