[libcxx-commits] [PATCH] D94544: [libc++] NFCI: Refactor allocator_traits
Zoe Carver via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Jan 12 14:55:25 PST 2021
zoecarver added a comment.
Thanks for the cleanup, this is much improved!
================
Comment at: libcxx/include/__memory/allocator_traits.h:154
+_LIBCPP_SUPPRESS_DEPRECATED_PUSH
+_LIBCPP_ALLOCATOR_TRAITS_HAS_XXX_TEMPLATE(__has_rebind_other, _Up, rebind<_Up>::other);
template <class _Tp, class _Up, bool = __has_rebind_other<_Tp, _Up>::value>
----------------
If this is the only use of `_LIBCPP_ALLOCATOR_TRAITS_HAS_XXX_TEMPLATE`, is it better to just implement it here (and get rid of the macro)?
================
Comment at: libcxx/include/__memory/allocator_traits.h:173
+#undef _LIBCPP_ALLOCATOR_TRAITS_HAS_XXX
+#undef _LIBCPP_ALLOCATOR_TRAITS_HAS_XXX_TEMPLATE
----------------
Let's put these at the bottom of the file. Otherwise, if someone tries to use them later on it might create a hard-to-fix error.
================
Comment at: libcxx/include/__memory/allocator_traits.h:245
+ template <class _Tp>
+ using rebind_alloc = typename __allocator_traits_rebind<allocator_type, _Tp>::type;
+ template <class _Tp>
----------------
Could this be `__allocator_traits_rebind_t`?
================
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
----------------
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.
================
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)...);
----------------
I don't really understand why we have two implementations here. This isn't a constexpr context, so they both do the same thing.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D94544/new/
https://reviews.llvm.org/D94544
More information about the libcxx-commits
mailing list