This is the mail archive of the
libstdc++@gcc.gnu.org
mailing list for the libstdc++ project.
Re: debug container patch
- From: Jonathan Wakely <jwakely at redhat dot com>
- To: François Dumont <frs dot dumont at gmail dot com>
- Cc: "libstdc++ at gcc dot gnu dot org" <libstdc++ at gcc dot gnu dot org>, gcc-patches <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 6 May 2014 15:05:21 +0100
- Subject: Re: debug container patch
- Authentication-results: sourceware.org; auth=none
- References: <53503D01 dot 5000007 at gmail dot com> <20140427133918 dot GV928 at redhat dot com> <535EC323 dot 2050108 at gmail dot com>
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.*/
+ 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.
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.