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 Fri, Jul 17, 2015 at 7:16 PM, Fan You <> wrote:
> Hi,
> 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.

> [8.3] Type-Erased allocator

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

Format: please replace leading consecutive spaces with tabs.

    static std::atomic<memory_resource*> s_default_resource;

naming: _S_default_resource.

    virtual ~memory_resource() { }

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

  template <typename _Tp>
    class __constructor_helper_imp

This doesn't work correctly for at least following case:

Based on [], 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,
    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.

  bool operator==(const memory_resource& __a,
          const memory_resource& __b) noexcept
  { return &__a == &__b || __a.is_equal(__b); }

Make all non-template functions inlined.

      { } // used here

What does this mean?

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

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

      memory_resource* _M_resource;

private member variables should be defined after private member functions.

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

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

What if the user turns off RTTI?

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

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

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

    memory_resource* __prev = get_default_resource();;
    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.

Tim Shen

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