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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a subscriber: libc++ vendors.
ldionne marked an inline comment as done.
ldionne added inline comments.


================
Comment at: libcxx/docs/ReleaseNotes.rst:68-71
+- The extension for ``std::allocator`` to support ``const`` types has been removed.
+  In particular, this means that instantiating most container types with const-qualified
+  types will not work anymore (for example ``std::vector<const int>`` won't work anymore).
+  If you were using that pattern, please use a non const-qualified type instead.
----------------
Quuxplusone wrote:
> I think this buries the lede a little bit. I'd say more like
> ```
> - libc++ no longer supports containers of ``const``-qualified element type,
>   such as ``vector<const T>`` and ``list<const T>``. This used to be supported
>   as an extension. Likewise, ``std::allocator<const T>`` is no longer supported.
>   If you were using ``vector<const T>``, replace it with ``vector<T>`` instead.
> ```
> I actually wonder whether this change matters to `list<const T>`, since it's a node-based container. Should you add `libcxx/test/libcxx/.../const_element_type.verify.cpp` tests that check what happens with const-qualified element types? If `list<const T>` and `set<const T>` still work in practice, should we continue kinda-sorta-supporting them, and document here that `vector` and `deque` are the only two containers that have been de-supported?
[Sorry, it seems I didn't publish this reply the first time around]

I had to remove one instance of `list<const T>` in the tests because it broke, so I guess it means it doesn't work anymore either. Which makes sense, since `std::allocator<const T>` is not supposed to work, period. So regardless of the fact that `std::list` will rebind the allocator to the node type, you're still technically mentioning `std::allocator<const T>`, and so it's not valid.

I will reword the release note.


================
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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120996