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