Re: [GSoC] Patches for shared_ptr array and polymorphic_allocator

[Jonathan Wakely <jwakely at redhat dot com>]

On 18/07/15 00:02 -0700, Tim Shen wrote:
> On Fri, Jul 17, 2015 at 7:16 PM, Fan You <youfan.noey@gmail.com> wrote:
> > Hi,
> >
> > According to <http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4335.html#memory.smartptr>
> >
> > Here is my implementation of
> >
> > [8.2] Extend shared_ptr to support arrays
>
> Please don't resend the shared_ptr patch, since it's already tracked
> in another thread.

Right, we can keep the two pieces of work entirely separate, I think.

> > [8.3] Type-Erased allocator
>
> Please send a working patch with tests and (probably with Makefile.am changes).

Yes, the code looks really good but we need tests to know it works.

Tim's review is great, I will just add a few more comments.

> #ifndef _GLIBCXX_MEMORY_RESOURCE
> #define _GLIBCXX_MEMORY_RESOURCE 1

Please make this _GLIBCXX_EXPERIMENTAL_MEMORY_RESOURCE so that it
doesn't need to be renamed if the header is added to a later standard
and we have both <memory_resource> and <experimental/memory_resource>.

> // Decleartion
>  class memory_resource;

I don't think we need this comment, especially spelled wrong :-)

> L70:
>    static std::atomic<memory_resource*> s_default_resource;
>
> naming: _S_default_resource.

Similarly, dont_care_type is not a reserved name, so the header could
not be used in this legal program:

#define dont_care_type "oops this isn't going to compile"
#include <experimental/memory_resource>

However, I think the dont_care_type and __constructor_helper should go
away completely, see below.

Also, the TS says "The name resource_adaptor_imp is for exposition
only and is not normative," which means we cannot use that name,
because users could define it as a macro. I suggest changing it to
__resource_adaptor_impl or similar.


> L43:
>    virtual ~memory_resource() { }
>
> Please break the line after virtual/return type. This also applies for
> other places in the patch.

Not essential for 'virtual' or generally for inline functions that fit
on a single line, so this is OK to leave as it is.

> Consider use a more readable helper name, like
> __uses_allocator_construction_helper and document it.

I don't think the helper type is needed at all. It is performing the
same job as the helper types in <bits/uses_alloc.h>, which I see get
used, but not correctly.

The point of the __uses_alloc0, __uses_alloc1 and __uses_alloc2 tag
types is that you overload on them to perform the correct type of
initialization according to the tag type.

The second construct() overload:

     // Specializations for pair using piecewise construction
     template <typename _Tp1, typename _Tp2,
              typename... _Args1, typename... _Args2>
       void construct(pair<_Tp1, _Tp2>* __p, piecewise_construct_t,
                      tuple<_Args1...> __x,
                      tuple<_Args2...> __y)

should not use __constructor_helper at all, because you never need to
pass an allocator to std::pair, instead you (conditionally) pass an
allocator to its members, but that's done by _M_construct_p(). So you
should not be using __constructor_helper for this overload. Just
construct a std::pair directly.

And the first construct() overload:

     template <typename _Tp1, typename... _Args> //used here
       void construct(_Tp1* __p, _Args&&... __args)
       {
         using _Ctor_imp = __constructor_helper<_Tp1>;
         ::new ((void*)__p) _Ctor_imp(allocator_arg,
                                      this->resource(),
                                      std::forward<_Args>(__args)...);
       }

also doesn't need __constructor_helper if you follow the same design
as used in <scoped_allocator> and dispatch to another function
(called _M_construct in std::scoped_allocator_adaptor) using the
appropriate __use_alloc tag type.

Implementing uses-allocator construction that way is consistent with
our existing code and will work for this type, which I think would
fail with your __constructor_helper because only polymorphic_allocator
can call its constructor:

 class Unfriendly
 {
 private:
   Unfriendly() = default;

   template<typename T> friend class polymorphic_allocator;
 };

Also in regard to the __uses_allocX tag types, these functions:

     template<typename... _Args>
       decltype(auto)
       _M_construct_p(__uses_alloc1_, tuple<_Args...>& __t)
       { return tuple_cat(make_tuple(allocator_arg, this->resources()),
                          std::move(__t)); }

     template<typename... _Args>
       decltype(auto)
       _M_construct_p(__uses_alloc2_, tuple<_Args...>& __t)
       { return tuple_cat(std::move(__t), make_tuple(this->resources())); }

could use the _M_a member of the tag type to get the resource, instead
of this->resources(). That will actually compile, because the member
function is called resource() not resources() (so I assume these
functions have never been tested :-)

All the calls to tuple_cat and make_tuple must be qualified with std::
so that they do not use ADL.


> L73:
>  bool operator==(const memory_resource& __a,
>          const memory_resource& __b) noexcept
>  { return &__a == &__b || __a.is_equal(__b); }
>
> Make all non-template functions inlined.

Yes, this will cause a linker error if two different translation
units both include <experimental/memory_resource> because the function
will be defined twice.

> L340:
>    auto __p = dynamic_cast<const resource_adaptor_imp*>(&__other);
>
> What if the user turns off RTTI?

As this is only a TS we could just say that RTTI is required to use
polymorphic_allocator, but ideally we would have an alternative
implementation that works when __cpp_rtii is not defined. That can
probably be added later, so this is OK for now.


> L355:
>  // Global memory resources
>  atomic<memory_resource*> memory_resource::s_default_resource;
> L386:
>  // The default memory resource
>  memory_resource* get_default_resource() noexcept
>  {
>    memory_resource *__ret
>      = memory_resource::s_default_resource.load();
>
>    if (__ret == nullptr) { __ret = new_delete_resource(); }
>    return __ret;
>  }
>
> According to [8.8.5], memory resource pointer should be intialized
> with new_delete_resource(), not nullptr; and get_default_resource
> should only return the pointer.

Yes, I think this should be simply:

 inline memory_resource*
 get_default_resource() noexcept
 { return memory_resource::_S_default_resource.load(); }

> L396:
>  memory_resource* set_default_resource(memory_resource* __r) noexcept
>  {
>    if ( __r == nullptr)
>    { __r = new_delete_resource(); }

We never use this style of brace-placement. Either put the braces on
their own lines, indented by two spaces, or leave them out.

>    memory_resource* __prev = get_default_resource();
>    memory_resource::s_default_resource.store(__r);
>    return __prev;
>  }
>
> We shouldn't care if it's nullptr or not.

[memory.resource.global] p7 says we should care.

> Your get-then-set may cause a data race. I think
> std::atomic<>::exchange will work, but we should confirm with Jon.

Yes it's a race (although not one that results in undefined
behaviour). It should be:

 inline memory_resource*
 set_default_resource(memory_resource* __r) noexcept
 {
   if ( __r == nullptr)
     __r = new_delete_resource();
   return memory_resource::_S_default_resource.exchange(__r);
 }