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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision.
Quuxplusone added inline comments.


================
Comment at: libcxx/include/__memory/shared_ptr.h:285
 {
+    using _TpAlloc = typename __allocator_traits_rebind<_Alloc, typename remove_cv<_Tp>::type>::type;
+    using _ControlBlockAlloc = typename __allocator_traits_rebind<_Alloc, __shared_ptr_emplace>::type;
----------------
AFAICT, we never used to instantiate `TheUsersAllocator<_Tp>` in pre-C++20 modes; this diff makes us instantiate that type. Pathological user-defined allocators might explode at that, so we might want to avoid the instantiation as a QoI issue. However, given that
- we //already// instantiate that type in C++20, so this is just front-loading the pain (if any),
- hopefully you'll quickly do a followup PR to eliminate the instantiation of `_TpAlloc` entirely,
I think this is fine.



================
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)...);
----------------
ldionne wrote:
> 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.
> However, prior to C++11, std::allocator did use pointer instead of `T*`

Ah. Well, that's definitely a good enough reason in C++03 codepaths, then... but this codepath is under `_LIBCPP_STD_VER > 17`! So yeah, I think it would be nice to change (and don't object to making that a separate PR if that's what you want).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120996