This is the mail archive of the 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: Add std::unordered_* C++11 allocator support

On 2 April 2013 21:39, François Dumont wrote:
> Here is an updated, exception safe, version.
> To validate it I have implemented a number of enhancements to the exception
> test framework, mostly to the basic safety test:
> - The allocator label is incremented throughout all tested methods rather
> than reseted to 0 before each method. Adding more tests I had the problem of
> a container containing memory allocated through a method with label N and
> detected as memory leak when checking an other method later.
> - I have added a check that there is no memory allocated left after all
> methods got tested. To make this assumption sure I prefer to make test_base
> a stateless class and instantiate everything in the run method. This way,
> once outside scope of instantiation, there should be nothing left.
> - I have added check on correct invocation of allocator construct/destroy.
> It is limited to C++11 mode, I will check if I can remove this limitation.

That's a good thing to test for, nice work.  I'm OK with the
limitation, but if you want to remove it that's fine.

> - I have added check of copy assignment and move assignment for basic
> exception safety

I noticed this in the new throw_allocator code:

+#if __cplusplus >= 201103L
+      {
+    auto beg = map_construct().begin();
+    auto end = map_construct().end();
+    while (beg != end)
+      {
+        if (beg->second == label)
+          log_to_string(found, *beg);
+        ++beg;
+      }
+      }

That would be simpler with the new for-loop:

for (auto& e : map_construct())
  if (e.second == label)
    log_to_string(found, e);

That's inconsistent with the other loops though, so I don't mind which is used.

>     I plan to add a state information on the throw_allocator_base so that I
> can test exception safety on operations dealing with not equal allocators,
> does it sound ok ?

Yes, I think so.  Testing stateful allocators is as important now.

>     I also face an issue regarding implementation of copy assignment
> operator. When allocators are not equal we need to release memory before
> reallocating everything once __alloc_on_copy has been called. The problem is
> that if this method or the allocation following throw we end up with a
> _Hashtable instance having no memory allocated. I prefer not to call
> __alloc_on_copy the other way and prepare the _Hashtable for this situation.
> Some code was not expecting 0 buckets, like the % operation, so I had to
> make _Hashtable ready for that adding some __builtin_except calls. Doing so
> I have been able to leave the _Hashtable instance alloc free after a move
> and not reallocate a small number of buckets to surely free it just a little
> bit later.
> Let me know what you think about it.

_ReuseOrAllocNode has no comments documenting its purpose.

I'm not very comfortable with the "mutable value type" part which
accesses the stored value through a pointer to a different type. This
violates strict aliasing rules and would also give the wrong result if
users provide an explicit specialization of std::pair<int, Foo> which
stores pair::second before pair::first, then tries to store that type
in an unordered_map.  When a pair<const int, Foo>* is cast to
pair<int, Foo>* the user's explicit specialization would not be used
and the layout would be wrong.  Would it be possible to separate the
C++11 allocator support from the new feature that attempts to reuse
existing nodes?  Or maybe remove the mutable value type stuff so that
nodes can be reused for unordered sets but not unordered maps?

+      typedef typename _Value_alloc_traits::reference        reference;
+      typedef typename _Value_alloc_traits::const_reference    const_reference;

This part of the change should be just:

      typedef value_type&        reference;
      typedef const value_type&    const_reference;

as specified in the standard. (I realise this was probably copied from
forward_list, but I got it wrong there and will fix it.)

C++11 allocators do not define nested reference and const_reference
members, so the old code was 100% wrong to use them.  I added
reference and const_reference to the __alloc_traits class template so
that the other containers containers can use __alloc_traits::reference
in both C++03 and C++11 mode and get the right result (i.e. use
Alloc::reference in C++03 mode and use value_type& in C++11 mode).  In
C++11-only containers we don't need that portability shim and can just
use value_type& directly.

If _M_bucket_count can be zero now, can we make the move constructor noexcept?

I haven't finished reviewing the patch yet, I'll continue later.

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