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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

I'll try landing this again now, since CI is green and I've added tests + fixed the issues reported by @phosek.



================
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:
> > > 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).
Alright, will do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120996