Re: [GSoC] Patches for shared_ptr array and polymorphic_allocator

[Tim Shen <timshen at google dot com>]

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.

> [8.3] Type-Erased allocator

Please send a working patch with tests and (probably with Makefile.am changes).

Format: please replace leading consecutive spaces with tabs.

L70:
    static std::atomic<memory_resource*> s_default_resource;

naming: _S_default_resource.

L43:
    virtual ~memory_resource() { }

Please break the line after virtual/return type. This also applies for
other places in the patch.

L81:
  template <typename _Tp>
    class __constructor_helper_imp

This doesn't work correctly for at least following case:
    std::__constructor_helper_imp<int*>(std::allocator_arg,
                    std::allocator<int>(),
                    std::true_type(),
                    std::true_type(),
                    std::true_type());

Based on [allocator.uses.construction], this falls into the second
rule, because there exists priorities in those rules; but overloading
resolution thinks it's ambiguous.

To enforce the order, you can do this:

template<int __rule_num>
struct __uses_allocator_construction_helper;

...
{
    constexpr bool __uses_alloc = uses_allocator<...>::value;
    constexpr bool __normally_constructible = is_constructible<_Tp,
_Args...>::value;
    constexpr bool __constructible_alloc_before =
is_constructible<_Tp, allocator_tag_t, _Alloc, _Args...>::value;
    constexpr bool __constructible_alloc_after =
is_constructible<_Args..., _Alloc>::value;

    constexpr bool __uses_rule1 = !__uses_alloc && __normally_constructible;
    constexpr bool __uses_rule2 = __uses_alloc && __constructible_alloc_before;
    constexpr bool __uses_rule3 = __uses_alloc && __constructible_alloc_after;
    __uses_allocator_construction_helper<__uses_rule1 ? 1 :
(__uses_rule2 ? 2 : (__uses_rule3 ? 3 : 0))>::_S_apply(...);
}

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

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.

L178:
      { } // used here

What does this mean?

L180:
      polymorphic_allocator(memory_resource* __r)
      : _M_resource(__r ? __r : get_default_resource())
      { }

[8.6.2.3] describes __r != nullptr as a precondition, which is
guaranteed by the caller, so we don't have to check here.
Alternatively you can use _GLIBCXX_ASSERT.

L262:
      memory_resource* _M_resource;

private member variables should be defined after private member functions.

L286:
  template <class _Tp1, class _Tp2>
    bool operator!=(const polymorphic_allocator<_Tp1>& __a,
            const polymorphic_allocator<_Tp2>& __b) noexcept
    { return ! (__a == __b); }

Remove extra space after "!".

L340:
    auto __p = dynamic_cast<const resource_adaptor_imp*>(&__other);

What if the user turns off RTTI?

L345:
      // Calculate Aligned Size
      size_t _Aligned_size(size_t __size, size_t __alignment)
      { return ((__size - 1)|(__alignment - 1)) + 1; }

      bool _M_supported (size_t __x)
      { return ((__x != 0) && (__x != 0) && !(__x & (__x - 1))); }

Document these two functions' behaviors, e.g.:
Returns a size that is larger than or equal to __size and divided by
__alignment, where __alignment is required to be the power of 2.

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.

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

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


-- 
Regards,
Tim Shen