This is the mail archive of the libstdc++@gcc.gnu.org mailing list for the libstdc++ project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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.*/
+   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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]