[libc++] Add __detected_or_t and use it to implement some of the allocator traits aliases

[philnik777]

This simplifies the implementation a bit, since we don't need a lot of the __has_x classes anymore. We just need two template aliases to implement the allocator_traits aliases now.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

This simplifies the implementation a bit, since we don't need a lot of the __has_x classes anymore. We just need two template aliases to implement the allocator_traits aliases now.

---

Full diff: https://github.com/llvm/llvm-project/pull/115654.diff

5 Files Affected:

• (modified) libcxx/include/CMakeLists.txt (+1)
• (modified) libcxx/include/__memory/allocator_traits.h (+40-55)
• (modified) libcxx/include/__memory/unique_ptr.h (+2-2)
• (added) libcxx/include/__type_traits/detected_or.h (+36)
• (modified) libcxx/include/module.modulemap (+1)

diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index dd774b25a81eda..98b9a19e4227ef 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -758,6 +758,7 @@ set(files
   __type_traits/decay.h
   __type_traits/dependent_type.h
   __type_traits/desugars_to.h
+  __type_traits/detected_or.h
   __type_traits/disjunction.h
   __type_traits/enable_if.h
   __type_traits/extent.h
diff --git a/libcxx/include/__memory/allocator_traits.h b/libcxx/include/__memory/allocator_traits.h
index 499b30b85b6c9b..62b454c9227523 100644
--- a/libcxx/include/__memory/allocator_traits.h
+++ b/libcxx/include/__memory/allocator_traits.h
@@ -15,6 +15,7 @@
 #include <__fwd/memory.h>
 #include <__memory/construct_at.h>
 #include <__memory/pointer_traits.h>
+#include <__type_traits/detected_or.h>
 #include <__type_traits/enable_if.h>
 #include <__type_traits/is_constructible.h>
 #include <__type_traits/is_empty.h>
@@ -42,17 +43,11 @@ _LIBCPP_BEGIN_NAMESPACE_STD
   struct NAME<_Tp, __void_t<typename _Tp::PROPERTY > > : true_type {}
 
 // __pointer
-template <class _Tp,
-          class _Alloc,
-          class _RawAlloc = __libcpp_remove_reference_t<_Alloc>,
-          bool            = __has_pointer<_RawAlloc>::value>
-struct __pointer {
-  using type _LIBCPP_NODEBUG = typename _RawAlloc::pointer;
-};
-template <class _Tp, class _Alloc, class _RawAlloc>
-struct __pointer<_Tp, _Alloc, _RawAlloc, false> {
-  using type _LIBCPP_NODEBUG = _Tp*;
-};
+template <class _Tp>
+using __pointer_member = typename _Tp::pointer;
+
+template <class _Tp, class _Alloc>
+using __pointer = __detected_or_t<_Tp*, __pointer_member, __libcpp_remove_reference_t<_Alloc> >;
 
 // __const_pointer
 _LIBCPP_ALLOCATOR_TRAITS_HAS_XXX(__has_const_pointer, const_pointer);
@@ -100,13 +95,11 @@ struct __const_void_pointer<_Ptr, _Alloc, false> {
 };
 
 // __size_type
-_LIBCPP_ALLOCATOR_TRAITS_HAS_XXX(__has_size_type, size_type);
-template <class _Alloc, class _DiffType, bool = __has_size_type<_Alloc>::value>
-struct __size_type : make_unsigned<_DiffType> {};
+template <class _Tp>
+using __size_type_member = typename _Tp::size_type;
+
 template <class _Alloc, class _DiffType>
-struct __size_type<_Alloc, _DiffType, true> {
-  using type _LIBCPP_NODEBUG = typename _Alloc::size_type;
-};
+using __size_type = __detected_or_t<__make_unsigned_t<_DiffType>, __size_type_member, _Alloc>;
 
 // __alloc_traits_difference_type
 _LIBCPP_ALLOCATOR_TRAITS_HAS_XXX(__has_alloc_traits_difference_type, difference_type);
@@ -120,40 +113,34 @@ struct __alloc_traits_difference_type<_Alloc, _Ptr, true> {
 };
 
 // __propagate_on_container_copy_assignment
-_LIBCPP_ALLOCATOR_TRAITS_HAS_XXX(__has_propagate_on_container_copy_assignment, propagate_on_container_copy_assignment);
-template <class _Alloc, bool = __has_propagate_on_container_copy_assignment<_Alloc>::value>
-struct __propagate_on_container_copy_assignment : false_type {};
+template <class _Tp>
+using __propagate_on_container_copy_assignment_member = typename _Tp::propagate_on_container_copy_assignment;
+
 template <class _Alloc>
-struct __propagate_on_container_copy_assignment<_Alloc, true> {
-  using type _LIBCPP_NODEBUG = typename _Alloc::propagate_on_container_copy_assignment;
-};
+using __propagate_on_container_copy_assignment =
+    __detected_or_t<false_type, __propagate_on_container_copy_assignment_member, _Alloc>;
 
 // __propagate_on_container_move_assignment
-_LIBCPP_ALLOCATOR_TRAITS_HAS_XXX(__has_propagate_on_container_move_assignment, propagate_on_container_move_assignment);
-template <class _Alloc, bool = __has_propagate_on_container_move_assignment<_Alloc>::value>
-struct __propagate_on_container_move_assignment : false_type {};
+template <class _Tp>
+using __propagate_on_container_move_assignment_member = typename _Tp::propagate_on_container_move_assignment;
+
 template <class _Alloc>
-struct __propagate_on_container_move_assignment<_Alloc, true> {
-  using type _LIBCPP_NODEBUG = typename _Alloc::propagate_on_container_move_assignment;
-};
+using __propagate_on_container_move_assignment =
+    __detected_or_t<false_type, __propagate_on_container_move_assignment_member, _Alloc>;
 
 // __propagate_on_container_swap
-_LIBCPP_ALLOCATOR_TRAITS_HAS_XXX(__has_propagate_on_container_swap, propagate_on_container_swap);
-template <class _Alloc, bool = __has_propagate_on_container_swap<_Alloc>::value>
-struct __propagate_on_container_swap : false_type {};
+template <class _Tp>
+using __propagate_on_container_swap_member = typename _Tp::propagate_on_container_swap;
+
 template <class _Alloc>
-struct __propagate_on_container_swap<_Alloc, true> {
-  using type _LIBCPP_NODEBUG = typename _Alloc::propagate_on_container_swap;
-};
+using __propagate_on_container_swap = __detected_or_t<false_type, __propagate_on_container_swap_member, _Alloc>;
 
 // __is_always_equal
-_LIBCPP_ALLOCATOR_TRAITS_HAS_XXX(__has_is_always_equal, is_always_equal);
-template <class _Alloc, bool = __has_is_always_equal<_Alloc>::value>
-struct __is_always_equal : is_empty<_Alloc> {};
+template <class _Tp>
+using __is_always_equal_member = typename _Tp::is_always_equal;
+
 template <class _Alloc>
-struct __is_always_equal<_Alloc, true> {
-  using type _LIBCPP_NODEBUG = typename _Alloc::is_always_equal;
-};
+using __is_always_equal = __detected_or_t<typename is_empty<_Alloc>::type, __is_always_equal_member, _Alloc>;
 
 // __allocator_traits_rebind
 _LIBCPP_SUPPRESS_DEPRECATED_PUSH
@@ -245,20 +232,18 @@ _LIBCPP_CTAD_SUPPORTED_FOR_TYPE(allocation_result);
 
 template <class _Alloc>
 struct _LIBCPP_TEMPLATE_VIS allocator_traits {
-  using allocator_type     = _Alloc;
-  using value_type         = typename allocator_type::value_type;
-  using pointer            = typename __pointer<value_type, allocator_type>::type;
-  using const_pointer      = typename __const_pointer<value_type, pointer, allocator_type>::type;
-  using void_pointer       = typename __void_pointer<pointer, allocator_type>::type;
-  using const_void_pointer = typename __const_void_pointer<pointer, allocator_type>::type;
-  using difference_type    = typename __alloc_traits_difference_type<allocator_type, pointer>::type;
-  using size_type          = typename __size_type<allocator_type, difference_type>::type;
-  using propagate_on_container_copy_assignment =
-      typename __propagate_on_container_copy_assignment<allocator_type>::type;
-  using propagate_on_container_move_assignment =
-      typename __propagate_on_container_move_assignment<allocator_type>::type;
-  using propagate_on_container_swap = typename __propagate_on_container_swap<allocator_type>::type;
-  using is_always_equal             = typename __is_always_equal<allocator_type>::type;
+  using allocator_type                         = _Alloc;
+  using value_type                             = typename allocator_type::value_type;
+  using pointer                                = __pointer<value_type, allocator_type>;
+  using const_pointer                          = typename __const_pointer<value_type, pointer, allocator_type>::type;
+  using void_pointer                           = typename __void_pointer<pointer, allocator_type>::type;
+  using const_void_pointer                     = typename __const_void_pointer<pointer, allocator_type>::type;
+  using difference_type                        = typename __alloc_traits_difference_type<allocator_type, pointer>::type;
+  using size_type                              = __size_type<allocator_type, difference_type>;
+  using propagate_on_container_copy_assignment = __propagate_on_container_copy_assignment<allocator_type>;
+  using propagate_on_container_move_assignment = __propagate_on_container_move_assignment<allocator_type>;
+  using propagate_on_container_swap            = __propagate_on_container_swap<allocator_type>;
+  using is_always_equal                        = __is_always_equal<allocator_type>;
 
 #ifndef _LIBCPP_CXX03_LANG
   template <class _Tp>
diff --git a/libcxx/include/__memory/unique_ptr.h b/libcxx/include/__memory/unique_ptr.h
index 28c62e13566e24..9526255583dd56 100644
--- a/libcxx/include/__memory/unique_ptr.h
+++ b/libcxx/include/__memory/unique_ptr.h
@@ -143,7 +143,7 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr {
 public:
   typedef _Tp element_type;
   typedef _Dp deleter_type;
-  typedef _LIBCPP_NODEBUG typename __pointer<_Tp, deleter_type>::type pointer;
+  using pointer _LIBCPP_NODEBUG = __pointer<_Tp, deleter_type>;
 
   static_assert(!is_rvalue_reference<deleter_type>::value, "the specified deleter type cannot be an rvalue reference");
 
@@ -410,7 +410,7 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr<_Tp[], _Dp>
 public:
   typedef _Tp element_type;
   typedef _Dp deleter_type;
-  typedef typename __pointer<_Tp, deleter_type>::type pointer;
+  using pointer = __pointer<_Tp, deleter_type>;
 
   // A unique_ptr contains the following members which may be trivially relocatable:
   // - pointer: this may be trivially relocatable, so it's checked
diff --git a/libcxx/include/__type_traits/detected_or.h b/libcxx/include/__type_traits/detected_or.h
new file mode 100644
index 00000000000000..390f368411471e
--- /dev/null
+++ b/libcxx/include/__type_traits/detected_or.h
@@ -0,0 +1,36 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef _LIBCPP___TYPE_TRAITS_DETECTED_OR_H
+#define _LIBCPP___TYPE_TRAITS_DETECTED_OR_H
+
+#include <__config>
+#include <__type_traits/void_t.h>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#  pragma GCC system_header
+#endif
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+template <class _Default, class _Void, template <class...> class _Op, class... _Args>
+struct __detector {
+  using type = _Default;
+};
+
+template <class _Default, template <class...> class _Op, class... _Args>
+struct __detector<_Default, __void_t<_Op<_Args...> >, _Op, _Args...> {
+  using type = _Op<_Args...>;
+};
+
+template <class _Default, template <class...> class _Op, class... _Args>
+using __detected_or_t = typename __detector<_Default, void, _Op, _Args...>::type;
+
+_LIBCPP_END_NAMESPACE_STD
+
+#endif // _LIBCPP___TYPE_TRAITS_DETECTED_OR_H
diff --git a/libcxx/include/module.modulemap b/libcxx/include/module.modulemap
index 9027c28632dcda..92b90db816ab03 100644
--- a/libcxx/include/module.modulemap
+++ b/libcxx/include/module.modulemap
@@ -87,6 +87,7 @@ module std_core [system] {
     module decay                                      { header "__type_traits/decay.h" }
     module dependent_type                             { header "__type_traits/dependent_type.h" }
     module desugars_to                                { header "__type_traits/desugars_to.h" }
+    module detected_or                                { header "__type_traits/detected_or.h" }
     module disjunction                                { header "__type_traits/disjunction.h" }
     module enable_if                                  { header "__type_traits/enable_if.h" }
     module extent                                     { header "__type_traits/extent.h" }

[ldionne]

I like this a lot, nice simplification.

[dwblaikie]

FWIW, seeing some substantial (multiple single-digit % of overall linked binary size, double digit % growth of the DWARF index (.debug_gnu_pubnames/.gdb_index in our case, but I expect we'll see something similar in .debug_names too (FYI @adrian-prantl ))).

The specific data we have for an easily shareable example is (though I probably don't have an exhaustive list of flags here - an estimate of flags that may be relevant to the specific numbers):
clang, debug build (-g, -gz=zstd, -gsplit-dwarf, -ggnu-pubnames):
In .o/.dwo files:
+15% .debug_gnu_pubnames
+1.12% .debug_abbrev.dwo

clang, opt build (-g, -gz=zstd, -gsplit-dwarf, -ggnu-pubnames, -fdebug-types-section):
in .o/.dwo files:
+16% .debug_gnu_pubnames
+2.7% .group (more comdat groups that aren't optimized away at compile-time?)
+1-1.5% growth in: .debug_abbrev.dwo, .crel.debug_line, .crel.eh_frame, .debug_line_str, .debug_rnglists, .eh_frame, .debug_abbrev.dwo

(and including a 1.58% increase in executable size, including a 1.17% increase in .debug_rnglists and 6% increase in .gdb_index)

Looking into it further/will update with more info as I have it.

Maybe a lot of this could be addressed by adding the nodebug attribute to this __detected_or_t, but that probably wouldn't account for the rangelist increases or non-debug changes like .group.

[dwblaikie]

(oh, and @cjdb who had asked me about the nodebug attribute on type traits previously)

[ldionne]

Thanks for the heads up. Indeed, I strongly suspect that most of this would be alleviated by adding _LIBCPP_NODEBUG to the new aliases. If you are able to reproduce this locally, could you try it out to see how much that helps?

[dwblaikie]

Yeah (I was thinking of adding the attribute to __detected_or_t (& the __detected struct template (& its partial specialization)) itself, but yeah, any of the alias declarations should probably have it too (but its class/struct type information that tends to be much heavier weight than aliases/typedefs) if they're part of metaprogramming - they're probably not much use to people debugging code, I think (because you can see the specific thing they were resolved to easily)) - I've handed this off to @cmtice internally to try exactly that (applying the attribute to the __detected* stuff, and see if that accounts for most/all of the change)

I do worry a bit about the increase to non-debug (& debug-but-not-type) information, like .group (and .debug_rnglists).

[philnik777]

Is there a reason we should ever not put a _LIBCPP_NODEBUG on internal aliases?

I don't really see how this could have anything to do with anything but aliases, since the number of struct instantiations shouldn't change.

[dwblaikie]

Is there a reason we should ever not put a _LIBCPP_NODEBUG on internal aliases?

Don't think so?

I don't really see how this could have anything to do with anything but aliases, since the number of struct instantiations shouldn't change.

Instantiations don't directly cause DWARF to be emitted - the instantiation has to be referenced by something, so avoiding references to the struct instantiation can reduce the debug info size cost.

Specifically, putting _LIBCPP_NODEBUG on the type member of a type trait struct avoids needing to describe the struct (to put the type member inside of it), which saves a bunch of DWARF. Non-member aliases are nice to attribute too/nice to avoid describing in the DWARF - but the struct types are especially expensive/more verbose than non-member aliases.

[cmtice]

I have verified that adding _LIBCPP_NODEBUG on all the newly introduced 'using' statements, reduces the size increase (on my particular test case) from 9.5% down to 0.2%, Should I send a PR with this change?

[cmtice]

(I could add it to some of the other 'using' statements, too, and reduce it further)

[ldionne]

Yes, a PR would be great!

[cmtice]

Quick question: I don't usually work in libcxx: What's the best way to officially test my change? And do I need to try to write a test case for this?

[philnik777]

We don't have any infrastructure for this kind of test currently, so I don't think you need to add a test. If you have a good idea I'd be interested though.

Let's tackle the regression for now. If we have some general guidelines when to add [[gnu::nodebug]] we should add that in a separate PR. I've looked into adding a clang-tidy check to add the attribute to all libc++-internal aliases, so a discussion on any heuristic we can use would be nice.

[ldionne]

@cmtice If you open a PR, our CI will tell you if you messed up badly enough to break something else :). That should be sufficient for a patch of this kind.

[cmtice]

I've created a PR with my fixes: #118835