[libcxx-commits] [PATCH] D94544: [libc++] NFCI: Refactor allocator_traits

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/include/__memory/allocator_traits.h:273
+    template <class _Ap = _Alloc, class = void, class =
+        _EnableIf<!__has_allocate_hint<_Ap, size_type, const_void_pointer>::value> >
+    _LIBCPP_NODISCARD_AFTER_CXX17 _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
----------------
ldionne wrote:
> zoecarver wrote:
> > Why the has_allocate_hint? Is that just for the deprecation warnings? Is it worth just taking the deprecation warnings so we could have just one implementation? For compile-time if nothing else. Those SFINAE checks are slow. 
> Do you mean that you'd prefer doing the check for whether `a.allocate(n, hint)` is supported inline in the template declaration?
> 
> If you're asking why we call `allocate` with a hint at all, it's because we must. If a user-defined allocator does something special when called with a hint, we need to maintain that.
Oops, I didn't see that we're only passing the hint argument in one of these. Never mind then :)


================
Comment at: libcxx/include/__memory/allocator_traits.h:296
+    static void construct(allocator_type&, _Tp* __p, _Args&&... __args) {
 #if _LIBCPP_STD_VER > 17
+        _VSTD::construct_at(__p, _VSTD::forward<_Args>(__args)...);
----------------
ldionne wrote:
> zoecarver wrote:
> > I don't really understand why we have two implementations here. This isn't a constexpr context, so they both do the same thing.
> `construct_at` isn't provided before C++20.
I was thinking the opposite: always construct with placement new. Is there a way (that isn't UB) for a user to know the difference?

(This is also a super small thing, no need to address it, I know this is a bit out-of-scope.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94544