Skip to content

[libc++][NFC] Simplify pair a bit #96165

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

Merged
merged 1 commit into from
Jun 26, 2024
Merged

Conversation

philnik777
Copy link
Contributor

No description provided.

@philnik777 philnik777 marked this pull request as ready for review June 23, 2024 20:05
@philnik777 philnik777 requested a review from a team as a code owner June 23, 2024 20:06
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jun 23, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 23, 2024

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

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

1 Files Affected:

  • (modified) libcxx/include/__utility/pair.h (+5-12)
diff --git a/libcxx/include/__utility/pair.h b/libcxx/include/__utility/pair.h
index 1f5d19dd4c2b1..8872e5d9bef58 100644
--- a/libcxx/include/__utility/pair.h
+++ b/libcxx/include/__utility/pair.h
@@ -16,8 +16,6 @@
 #include <__fwd/array.h>
 #include <__fwd/pair.h>
 #include <__fwd/tuple.h>
-#include <__tuple/sfinae_helpers.h>
-#include <__tuple/tuple_element.h>
 #include <__tuple/tuple_indices.h>
 #include <__tuple/tuple_like_no_subrange.h>
 #include <__tuple/tuple_size.h>
@@ -130,20 +128,15 @@ struct _LIBCPP_TEMPLATE_VIS pair
     }
   };
 
-  template <bool _MaybeEnable>
-  using _CheckArgsDep _LIBCPP_NODEBUG =
-      typename conditional< _MaybeEnable, _CheckArgs, __check_tuple_constructor_fail>::type;
-
-  template <bool _Dummy = true, __enable_if_t<_CheckArgsDep<_Dummy>::__enable_default(), int> = 0>
-  explicit(!_CheckArgsDep<_Dummy>::__enable_implicit_default()) _LIBCPP_HIDE_FROM_ABI
-  _LIBCPP_CONSTEXPR pair() _NOEXCEPT_(
+  template <bool _Dummy = true, __enable_if_t<_Dummy && _CheckArgs::__enable_default(), int> = 0>
+  explicit(!_CheckArgs::__enable_implicit_default()) _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR pair() _NOEXCEPT_(
       is_nothrow_default_constructible<first_type>::value&& is_nothrow_default_constructible<second_type>::value)
       : first(), second() {}
 
-  template <bool _Dummy = true,
-            __enable_if_t<_CheckArgsDep<_Dummy>::template __is_pair_constructible<_T1 const&, _T2 const&>(), int> = 0>
+  template <bool _Dummy                                                                                          = true,
+            __enable_if_t<_Dummy && _CheckArgs::template __is_pair_constructible<_T1 const&, _T2 const&>(), int> = 0>
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 explicit(
-      !_CheckArgsDep<_Dummy>::template __is_implicit<_T1 const&, _T2 const&>()) pair(_T1 const& __t1, _T2 const& __t2)
+      !_CheckArgs::template __is_implicit<_T1 const&, _T2 const&>()) pair(_T1 const& __t1, _T2 const& __t2)
       _NOEXCEPT_(is_nothrow_copy_constructible<first_type>::value&& is_nothrow_copy_constructible<second_type>::value)
       : first(__t1), second(__t2) {}
 

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

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

LGTM if CI passes, but I am pretty sure the code was written that way because this was upsetting compilers at some point. The condition inside explicit is technically not dependent as written, and I think this was causing problems.

Again, if CI happy, I am happy.

@philnik777 philnik777 merged commit 54cb5ca into llvm:main Jun 26, 2024
3 of 5 checks passed
@philnik777 philnik777 deleted the simplify_pair branch June 26, 2024 08:51
@jyknight
Copy link
Member

I believe this change is broken. This following code now fails to compile, under clang -std=c++20 -stdlib=libc++, and I think it ought not to.

#include <utility>
#include <vector>

struct Test {
  std::vector<std::pair<int, Test>> v;
};

std::pair<int, Test> p;

jyknight added a commit that referenced this pull request Jun 28, 2024
ldionne pushed a commit that referenced this pull request Jun 28, 2024
Reverts #96165

The change broke code like

  #include <utility>
  #include <vector>

  struct Test {
    std::vector<std::pair<int, Test>> v;
  };

  std::pair<int, Test> p;

under `-std=c++20`, apparently by triggering certain template
evaluations too eagerly.
jyknight added a commit to jyknight/llvm-project that referenced this pull request Jul 1, 2024
PR llvm#96165 broke code similar to this test, and was subsequently
reverted. Add a test-case, to ensure the problem won't reoccur.
jyknight added a commit that referenced this pull request Jul 2, 2024
PR #96165 broke code similar to this test, and was subsequently
reverted. Add a test-case, to ensure the problem won't reoccur.

This error is potentially related to issues #59292 and #59966.
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
Reverts llvm#96165

The change broke code like

  #include <utility>
  #include <vector>

  struct Test {
    std::vector<std::pair<int, Test>> v;
  };

  std::pair<int, Test> p;

under `-std=c++20`, apparently by triggering certain template
evaluations too eagerly.
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this pull request Jul 3, 2024
PR llvm#96165 broke code similar to this test, and was subsequently
reverted. Add a test-case, to ensure the problem won't reoccur.

This error is potentially related to issues llvm#59292 and llvm#59966.
kbluck pushed a commit to kbluck/llvm-project that referenced this pull request Jul 6, 2024
PR llvm#96165 broke code similar to this test, and was subsequently
reverted. Add a test-case, to ensure the problem won't reoccur.

This error is potentially related to issues llvm#59292 and llvm#59966.
AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this pull request Jul 9, 2024
blueboxd pushed a commit to blueboxd/libcxx that referenced this pull request Apr 19, 2025
Reverts llvm/llvm-project#96165

The change broke code like

  #include <utility>
  #include <vector>

  struct Test {
    std::vector<std::pair<int, Test>> v;
  };

  std::pair<int, Test> p;

under `-std=c++20`, apparently by triggering certain template
evaluations too eagerly.

NOKEYCHECK=True
GitOrigin-RevId: 5e1de27f680591a870d78e9952b23f76aed7f456
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.

4 participants