Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc++] Remove <tuple> from <variant> #83183

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

philnik777
Copy link
Contributor

This moves a utility from <tuple> into an implementation detail header and refactors the selection of the variant index type to use.

@philnik777 philnik777 force-pushed the reduce_variant_include_size branch 3 times, most recently from dc157b3 to a3eba59 Compare March 8, 2024 19:04
Copy link
Member

@EricWF EricWF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo the one change with the metaprogramming.

if (__num_elem < numeric_limits<unsigned short>::max())
return 1;
return 2;
template <size_t _NumAlternatives>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider returning integral_constant<T, 0> so that we don't have to worry about type conversions, promotions, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is only an implementation detail for __variant_index_t this seems a bit overkill to me.

@philnik777 philnik777 force-pushed the reduce_variant_include_size branch from a3eba59 to f8079b8 Compare March 9, 2024 10:39
Copy link

github-actions bot commented Mar 9, 2024

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 4d323e404d43526ad8263769f00aace9db2e57c5 2eae390232b782124e2af254f188f12617abb2ef -- libcxx/include/__tuple/find_index.h libcxx/include/tuple libcxx/include/variant libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp libcxx/test/std/utilities/tuple/tuple.tuple/tuple.elem/tuple.by.type.verify.cpp libcxx/test/std/utilities/variant/variant.visit.member/visit.pass.cpp libcxx/test/std/utilities/variant/variant.visit.member/visit_return_type.pass.cpp libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp libcxx/test/std/utilities/variant/variant.visit/visit_return_type.pass.cpp
View the diff from clang-format here.
diff --git a/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.elem/tuple.by.type.verify.cpp b/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.elem/tuple.by.type.verify.cpp
index 00f27c3220..69ccb54d2a 100644
--- a/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.elem/tuple.by.type.verify.cpp
+++ b/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.elem/tuple.by.type.verify.cpp
@@ -18,13 +18,13 @@ struct UserType {};
 
 void test_bad_index() {
     std::tuple<long, long, char, std::string, char, UserType, char> t1;
-    TEST_IGNORE_NODISCARD std::get<int>(t1); // expected-error@*:* {{type not found}}
+    TEST_IGNORE_NODISCARD std::get<int>(t1);  // expected-error@*:* {{type not found}}
     TEST_IGNORE_NODISCARD std::get<long>(t1); // expected-note {{requested here}}
     TEST_IGNORE_NODISCARD std::get<char>(t1); // expected-note {{requested here}}
-        // expected-error@*:* 2 {{type occurs more than once}}
+                                              // expected-error@*:* 2 {{type occurs more than once}}
     std::tuple<> t0;
     TEST_IGNORE_NODISCARD std::get<char*>(t0); // expected-node {{requested here}}
-        // expected-error@*:* {{type not in empty type list}}
+                                               // expected-error@*:* {{type not in empty type list}}
 }
 
 void test_bad_return_type() {

@philnik777 philnik777 marked this pull request as ready for review March 11, 2024 13:04
@philnik777 philnik777 requested a review from a team as a code owner March 11, 2024 13:04
@philnik777 philnik777 force-pushed the reduce_variant_include_size branch from f8079b8 to 2eae390 Compare March 11, 2024 13:04
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Mar 11, 2024
@philnik777 philnik777 merged commit 2a38551 into llvm:main Mar 11, 2024
8 of 10 checks passed
@philnik777 philnik777 deleted the reduce_variant_include_size branch March 11, 2024 13:04
@llvmbot
Copy link
Member

llvmbot commented Mar 11, 2024

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

This moves a utility from &lt;tuple&gt; into an implementation detail header and refactors the selection of the variant index type to use.


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

14 Files Affected:

  • (modified) libcxx/include/CMakeLists.txt (+1)
  • (added) libcxx/include/__tuple/find_index.h (+62)
  • (modified) libcxx/include/libcxx.imp (+1)
  • (modified) libcxx/include/module.modulemap (+1)
  • (modified) libcxx/include/tuple (+1-34)
  • (modified) libcxx/include/variant (+18-13)
  • (modified) libcxx/test/libcxx/transitive_includes/cxx23.csv (-1)
  • (modified) libcxx/test/libcxx/transitive_includes/cxx26.csv (-1)
  • (modified) libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp (+7-7)
  • (modified) libcxx/test/std/utilities/tuple/tuple.tuple/tuple.elem/tuple.by.type.verify.cpp (+3-3)
  • (modified) libcxx/test/std/utilities/variant/variant.visit.member/visit.pass.cpp (+1)
  • (modified) libcxx/test/std/utilities/variant/variant.visit.member/visit_return_type.pass.cpp (+1)
  • (modified) libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp (+1)
  • (modified) libcxx/test/std/utilities/variant/variant.visit/visit_return_type.pass.cpp (+1)
diff --git a/libcxx/include/CMakeLists.txt b/libcxx/include/CMakeLists.txt
index e37c4ac4fddd8c..63adc03fae2980 100644
--- a/libcxx/include/CMakeLists.txt
+++ b/libcxx/include/CMakeLists.txt
@@ -701,6 +701,7 @@ set(files
   __thread/thread.h
   __thread/timed_backoff_policy.h
   __tree
+  __tuple/find_index.h
   __tuple/make_tuple_types.h
   __tuple/pair_like.h
   __tuple/sfinae_helpers.h
diff --git a/libcxx/include/__tuple/find_index.h b/libcxx/include/__tuple/find_index.h
new file mode 100644
index 00000000000000..133b00419d0c6c
--- /dev/null
+++ b/libcxx/include/__tuple/find_index.h
@@ -0,0 +1,62 @@
+//===----------------------------------------------------------------------===//
+//
+// 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___TUPLE_FIND_INDEX_H
+#define _LIBCPP___TUPLE_FIND_INDEX_H
+
+#include <__config>
+#include <__type_traits/is_same.h>
+#include <cstddef>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#  pragma GCC system_header
+#endif
+
+#if _LIBCPP_STD_VER >= 14
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+namespace __find_detail {
+
+static constexpr size_t __not_found = static_cast<size_t>(-1);
+static constexpr size_t __ambiguous = __not_found - 1;
+
+inline _LIBCPP_HIDE_FROM_ABI constexpr size_t __find_idx_return(size_t __curr_i, size_t __res, bool __matches) {
+  return !__matches ? __res : (__res == __not_found ? __curr_i : __ambiguous);
+}
+
+template <size_t _Nx>
+inline _LIBCPP_HIDE_FROM_ABI constexpr size_t __find_idx(size_t __i, const bool (&__matches)[_Nx]) {
+  return __i == _Nx
+           ? __not_found
+           : __find_detail::__find_idx_return(__i, __find_detail::__find_idx(__i + 1, __matches), __matches[__i]);
+}
+
+template <class _T1, class... _Args>
+struct __find_exactly_one_checked {
+  static constexpr bool __matches[sizeof...(_Args)] = {is_same<_T1, _Args>::value...};
+  static constexpr size_t value                     = __find_detail::__find_idx(0, __matches);
+  static_assert(value != __not_found, "type not found in type list");
+  static_assert(value != __ambiguous, "type occurs more than once in type list");
+};
+
+template <class _T1>
+struct __find_exactly_one_checked<_T1> {
+  static_assert(!is_same<_T1, _T1>::value, "type not in empty type list");
+};
+
+} // namespace __find_detail
+
+template <typename _T1, typename... _Args>
+struct __find_exactly_one_t : public __find_detail::__find_exactly_one_checked<_T1, _Args...> {};
+
+_LIBCPP_END_NAMESPACE_STD
+
+#endif // _LIBCPP_STD_VER >= 14
+
+#endif // _LIBCPP___TUPLE_FIND_INDEX_H
diff --git a/libcxx/include/libcxx.imp b/libcxx/include/libcxx.imp
index e02dc8da6ba182..b1e728cde868da 100644
--- a/libcxx/include/libcxx.imp
+++ b/libcxx/include/libcxx.imp
@@ -697,6 +697,7 @@
   { include: [ "<__thread/this_thread.h>", "private", "<thread>", "public" ] },
   { include: [ "<__thread/thread.h>", "private", "<thread>", "public" ] },
   { include: [ "<__thread/timed_backoff_policy.h>", "private", "<thread>", "public" ] },
+  { include: [ "<__tuple/find_index.h>", "private", "<tuple>", "public" ] },
   { include: [ "<__tuple/make_tuple_types.h>", "private", "<tuple>", "public" ] },
   { include: [ "<__tuple/pair_like.h>", "private", "<tuple>", "public" ] },
   { include: [ "<__tuple/sfinae_helpers.h>", "private", "<tuple>", "public" ] },
diff --git a/libcxx/include/module.modulemap b/libcxx/include/module.modulemap
index 98890e890cdb13..0bd2831b7f159c 100644
--- a/libcxx/include/module.modulemap
+++ b/libcxx/include/module.modulemap
@@ -1799,6 +1799,7 @@ module std_private_thread_thread               [system] {
 }
 module std_private_thread_timed_backoff_policy [system] { header "__thread/timed_backoff_policy.h" }
 
+module std_private_tuple_find_index       [system] { header "__tuple/find_index.h" }
 module std_private_tuple_make_tuple_types [system] { header "__tuple/make_tuple_types.h" }
 module std_private_tuple_pair_like        [system] {
   header "__tuple/pair_like.h"
diff --git a/libcxx/include/tuple b/libcxx/include/tuple
index 8808db6739fb9b..e63e4e25a7d2bd 100644
--- a/libcxx/include/tuple
+++ b/libcxx/include/tuple
@@ -213,6 +213,7 @@ template <class... Types>
 #include <__fwd/tuple.h>
 #include <__memory/allocator_arg_t.h>
 #include <__memory/uses_allocator.h>
+#include <__tuple/find_index.h>
 #include <__tuple/make_tuple_types.h>
 #include <__tuple/sfinae_helpers.h>
 #include <__tuple/tuple_element.h>
@@ -1087,40 +1088,6 @@ get(const tuple<_Tp...>&& __t) _NOEXCEPT {
 
 #  if _LIBCPP_STD_VER >= 14
 
-namespace __find_detail {
-
-static constexpr size_t __not_found = static_cast<size_t>(-1);
-static constexpr size_t __ambiguous = __not_found - 1;
-
-inline _LIBCPP_HIDE_FROM_ABI constexpr size_t __find_idx_return(size_t __curr_i, size_t __res, bool __matches) {
-  return !__matches ? __res : (__res == __not_found ? __curr_i : __ambiguous);
-}
-
-template <size_t _Nx>
-inline _LIBCPP_HIDE_FROM_ABI constexpr size_t __find_idx(size_t __i, const bool (&__matches)[_Nx]) {
-  return __i == _Nx
-           ? __not_found
-           : __find_detail::__find_idx_return(__i, __find_detail::__find_idx(__i + 1, __matches), __matches[__i]);
-}
-
-template <class _T1, class... _Args>
-struct __find_exactly_one_checked {
-  static constexpr bool __matches[sizeof...(_Args)] = {is_same<_T1, _Args>::value...};
-  static constexpr size_t value                     = __find_detail::__find_idx(0, __matches);
-  static_assert(value != __not_found, "type not found in type list");
-  static_assert(value != __ambiguous, "type occurs more than once in type list");
-};
-
-template <class _T1>
-struct __find_exactly_one_checked<_T1> {
-  static_assert(!is_same<_T1, _T1>::value, "type not in empty type list");
-};
-
-} // namespace __find_detail
-
-template <typename _T1, typename... _Args>
-struct __find_exactly_one_t : public __find_detail::__find_exactly_one_checked<_T1, _Args...> {};
-
 template <class _T1, class... _Args>
 inline _LIBCPP_HIDE_FROM_ABI constexpr _T1& get(tuple<_Args...>& __tup) noexcept {
   return std::get<__find_exactly_one_t<_T1, _Args...>::value>(__tup);
diff --git a/libcxx/include/variant b/libcxx/include/variant
index 5ce99250a8b4f4..d1eea52f0a9301 100644
--- a/libcxx/include/variant
+++ b/libcxx/include/variant
@@ -221,13 +221,18 @@ namespace std {
 #include <__functional/operations.h>
 #include <__functional/unary_function.h>
 #include <__memory/addressof.h>
+#include <__tuple/find_index.h>
+#include <__tuple/sfinae_helpers.h>
 #include <__type_traits/add_const.h>
 #include <__type_traits/add_cv.h>
 #include <__type_traits/add_pointer.h>
 #include <__type_traits/add_volatile.h>
+#include <__type_traits/common_type.h>
 #include <__type_traits/dependent_type.h>
 #include <__type_traits/is_array.h>
+#include <__type_traits/is_default_constructible.h>
 #include <__type_traits/is_destructible.h>
+#include <__type_traits/is_nothrow_assignable.h>
 #include <__type_traits/is_nothrow_move_constructible.h>
 #include <__type_traits/is_trivially_copy_assignable.h>
 #include <__type_traits/is_trivially_copy_constructible.h>
@@ -242,6 +247,7 @@ namespace std {
 #include <__utility/forward.h>
 #include <__utility/forward_like.h>
 #include <__utility/in_place.h>
+#include <__utility/integer_sequence.h>
 #include <__utility/move.h>
 #include <__utility/swap.h>
 #include <__variant/monostate.h>
@@ -249,7 +255,6 @@ namespace std {
 #include <initializer_list>
 #include <limits>
 #include <new>
-#include <tuple>
 #include <version>
 
 // standard-mandated includes
@@ -340,21 +345,20 @@ struct _LIBCPP_TEMPLATE_VIS variant_alternative<_Ip, variant<_Types...>> {
 
 inline constexpr size_t variant_npos = static_cast<size_t>(-1);
 
-_LIBCPP_HIDE_FROM_ABI constexpr int __choose_index_type(unsigned int __num_elem) {
-  if (__num_elem < numeric_limits<unsigned char>::max())
-    return 0;
-  if (__num_elem < numeric_limits<unsigned short>::max())
-    return 1;
-  return 2;
+template <size_t _NumAlternatives>
+_LIBCPP_HIDE_FROM_ABI constexpr auto __choose_index_type() {
+#ifdef _LIBCPP_ABI_VARIANT_INDEX_TYPE_OPTIMIZATION
+  if constexpr (_NumAlternatives < numeric_limits<unsigned char>::max())
+    return static_cast<unsigned char>(0);
+  else if constexpr (_NumAlternatives < numeric_limits<unsigned short>::max())
+    return static_cast<unsigned short>(0);
+  else
+#endif // _LIBCPP_ABI_VARIANT_INDEX_TYPE_OPTIMIZATION
+    return static_cast<unsigned int>(0);
 }
 
 template <size_t _NumAlts>
-using __variant_index_t =
-#  ifndef _LIBCPP_ABI_VARIANT_INDEX_TYPE_OPTIMIZATION
-    unsigned int;
-#  else
-    std::tuple_element_t< __choose_index_type(_NumAlts), std::tuple<unsigned char, unsigned short, unsigned int> >;
-#  endif
+using __variant_index_t = decltype(std::__choose_index_type<_NumAlts>());
 
 template <class _IndexType>
 constexpr _IndexType __variant_npos = static_cast<_IndexType>(-1);
@@ -1625,6 +1629,7 @@ _LIBCPP_POP_MACROS
 
 #if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 20
 #  include <exception>
+#  include <tuple>
 #  include <type_traits>
 #  include <typeinfo>
 #  include <utility>
diff --git a/libcxx/test/libcxx/transitive_includes/cxx23.csv b/libcxx/test/libcxx/transitive_includes/cxx23.csv
index 64ff9261820a96..436aa52b6de875 100644
--- a/libcxx/test/libcxx/transitive_includes/cxx23.csv
+++ b/libcxx/test/libcxx/transitive_includes/cxx23.csv
@@ -662,7 +662,6 @@ variant cstring
 variant initializer_list
 variant limits
 variant new
-variant tuple
 variant version
 vector array
 vector cctype
diff --git a/libcxx/test/libcxx/transitive_includes/cxx26.csv b/libcxx/test/libcxx/transitive_includes/cxx26.csv
index 64ff9261820a96..436aa52b6de875 100644
--- a/libcxx/test/libcxx/transitive_includes/cxx26.csv
+++ b/libcxx/test/libcxx/transitive_includes/cxx26.csv
@@ -662,7 +662,6 @@ variant cstring
 variant initializer_list
 variant limits
 variant new
-variant tuple
 variant version
 vector array
 vector cctype
diff --git a/libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp b/libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp
index 9011e61e78808a..2f1ea8bffb479b 100644
--- a/libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp
+++ b/libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp
@@ -49,13 +49,13 @@ void test_index_type() {
 template <class IndexType>
 void test_index_internals() {
   using Lim = std::numeric_limits<IndexType>;
-  static_assert(std::__choose_index_type(Lim::max() -1) !=
-                std::__choose_index_type(Lim::max()), "");
-  static_assert(std::is_same_v<
-      std::__variant_index_t<Lim::max()-1>,
-      std::__variant_index_t<Lim::max()>
-    > == ExpectEqual, "");
-  using IndexT = std::__variant_index_t<Lim::max()-1>;
+#ifdef _LIBCPP_ABI_VARIANT_INDEX_TYPE_OPTIMIZATION
+  static_assert(!std::is_same_v<decltype(std::__choose_index_type<Lim::max() - 1>()),
+                                decltype(std::__choose_index_type<Lim::max()>())>);
+#endif
+  static_assert(
+      std::is_same_v<std::__variant_index_t<Lim::max() - 1>, std::__variant_index_t<Lim::max()> > == ExpectEqual, "");
+  using IndexT   = std::__variant_index_t<Lim::max() - 1>;
   using IndexLim = std::numeric_limits<IndexT>;
   static_assert(std::__variant_npos<IndexT> == IndexLim::max(), "");
 }
diff --git a/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.elem/tuple.by.type.verify.cpp b/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.elem/tuple.by.type.verify.cpp
index 1d05eb5fe76e97..00f27c3220d2ed 100644
--- a/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.elem/tuple.by.type.verify.cpp
+++ b/libcxx/test/std/utilities/tuple/tuple.tuple/tuple.elem/tuple.by.type.verify.cpp
@@ -18,13 +18,13 @@ struct UserType {};
 
 void test_bad_index() {
     std::tuple<long, long, char, std::string, char, UserType, char> t1;
-    TEST_IGNORE_NODISCARD std::get<int>(t1); // expected-error@tuple:* {{type not found}}
+    TEST_IGNORE_NODISCARD std::get<int>(t1); // expected-error@*:* {{type not found}}
     TEST_IGNORE_NODISCARD std::get<long>(t1); // expected-note {{requested here}}
     TEST_IGNORE_NODISCARD std::get<char>(t1); // expected-note {{requested here}}
-        // expected-error@tuple:* 2 {{type occurs more than once}}
+        // expected-error@*:* 2 {{type occurs more than once}}
     std::tuple<> t0;
     TEST_IGNORE_NODISCARD std::get<char*>(t0); // expected-node {{requested here}}
-        // expected-error@tuple:* 1 {{type not in empty type list}}
+        // expected-error@*:* {{type not in empty type list}}
 }
 
 void test_bad_return_type() {
diff --git a/libcxx/test/std/utilities/variant/variant.visit.member/visit.pass.cpp b/libcxx/test/std/utilities/variant/variant.visit.member/visit.pass.cpp
index 68706d6c32af4f..50e7fc81387abc 100644
--- a/libcxx/test/std/utilities/variant/variant.visit.member/visit.pass.cpp
+++ b/libcxx/test/std/utilities/variant/variant.visit.member/visit.pass.cpp
@@ -21,6 +21,7 @@
 #include <cassert>
 #include <memory>
 #include <string>
+#include <tuple>
 #include <type_traits>
 #include <utility>
 #include <variant>
diff --git a/libcxx/test/std/utilities/variant/variant.visit.member/visit_return_type.pass.cpp b/libcxx/test/std/utilities/variant/variant.visit.member/visit_return_type.pass.cpp
index 20472c62fc5f98..b005f303bc4b6c 100644
--- a/libcxx/test/std/utilities/variant/variant.visit.member/visit_return_type.pass.cpp
+++ b/libcxx/test/std/utilities/variant/variant.visit.member/visit_return_type.pass.cpp
@@ -22,6 +22,7 @@
 #include <memory>
 #include <string>
 #include <type_traits>
+#include <tuple>
 #include <utility>
 #include <variant>
 
diff --git a/libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp b/libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp
index 097b784f2bf2ce..798ce7ded72a60 100644
--- a/libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp
+++ b/libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp
@@ -15,6 +15,7 @@
 #include <cassert>
 #include <memory>
 #include <string>
+#include <tuple>
 #include <type_traits>
 #include <utility>
 #include <variant>
diff --git a/libcxx/test/std/utilities/variant/variant.visit/visit_return_type.pass.cpp b/libcxx/test/std/utilities/variant/variant.visit/visit_return_type.pass.cpp
index eb425c07f93222..b1189dff656db4 100644
--- a/libcxx/test/std/utilities/variant/variant.visit/visit_return_type.pass.cpp
+++ b/libcxx/test/std/utilities/variant/variant.visit/visit_return_type.pass.cpp
@@ -15,6 +15,7 @@
 #include <cassert>
 #include <memory>
 #include <string>
+#include <tuple>
 #include <type_traits>
 #include <utility>
 #include <variant>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants