[GSoC] Patches for shared_ptr array and polymorphic_allocator

[Jonathan Wakely]

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);
  }