On 18/07/15 00:02 -0700, Tim Shen wrote:
On Fri, Jul 17, 2015 at 7:16 PM, Fan You youfan.n...@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::atomicmemory_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;
templatetypename T friend class polymorphic_allocator;
};
Also in regard to the __uses_allocX tag types, these functions:
templatetypename... _Args
decltype(auto)
_M_construct_p(__uses_alloc1_, tuple_Args... __t)
{ return tuple_cat(make_tuple(allocator_arg, this-resources()),
std::move(__t)); }
templatetypename... _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 ||
