[libcxx-commits] [PATCH] D120996: [libc++] Remove extension to support allocator<const T>

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__memory/shared_ptr.h:290-292
+        using _TpAlloc = typename __allocator_traits_rebind<_Alloc, remove_cv_t<_Tp>>::type;
         _TpAlloc __tmp(*__get_alloc());
         allocator_traits<_TpAlloc>::construct(__tmp, __get_elem(), _VSTD::forward<_Args>(__args)...);
----------------
Quuxplusone wrote:
> ldionne wrote:
> > Quuxplusone wrote:
> > > Pre-existing: Why do we rebind the allocator at all? `construct` is supposed to work on any allocator in the same family, isn't it? (IIRC, it deduces the type-to-construct from the pointer type of `__get_elem()`, not from the type of `__tmp`.) Is this a QoI thing, or just "historical reasons"?
> > I'm not sure I understand. `allocator_traits<>::construct(alloc, ptr, args...)` will call `alloc.construct(ptr, args...)`, and that requires `ptr` to be of type `alloc.value_type` [sic], doesn't it?
> No, `ptr` is of type `T*`, where `T` is deduced independently:
> https://en.cppreference.com/w/cpp/memory/allocator_traits/construct
> `allocate`/`deallocate` are `value_type`-dependent, but `construct`/`destroy` are heterogeneous. Yes this makes no sense, but it's how it has always been. (I assume there was an original bad rationale related to node-based containers.)
Hmm, I guess you're right. However, prior to C++11, `std::allocator` did use `pointer` instead of `T*`: https://en.cppreference.com/w/cpp/memory/allocator/construct

I suspect that is why it's like that. I'd be tempted to leave it as-is, at least in this patch. I can try to change it in a separate patch if you're curious.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120996/new/

https://reviews.llvm.org/D120996