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: [GSoC] Patches for shared_ptr array and polymorphic_allocator

On 18/07/15 00:02 -0700, Tim Shen wrote:
On Fri, Jul 17, 2015 at 7:16 PM, Fan You <> wrote:

According to <>

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 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.


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 :-)

   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.

   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,

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
   Unfriendly() = default;

   template<typename T> friend class polymorphic_allocator;

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

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

     template<typename... _Args>
       _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.

 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.

   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.

 // Global memory resources
 atomic<memory_resource*> memory_resource::s_default_resource;
 // 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(); }

 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();;
   return __prev;

We shouldn't care if it's nullptr or not.

[] 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();

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