Skip to content

[libcxx][test][NFC] Fix std::pair convertible tests in light of CWG2137 #97403

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
Jul 6, 2024

Conversation

MitalAshok
Copy link
Contributor

https://cplusplus.github.io/CWG/issues/2137.html

This change was previously made as part of 9247013 (#77768) and later reverted in 6e4930c

This change is still needed because the comment is still true: A standards-conformant compiler is currently supposed to fail this test.

This also means that any future work on CWG2137 with Clang would not need to modify the libc++ test suite

@MitalAshok MitalAshok requested a review from a team as a code owner July 2, 2024 10:07
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jul 2, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 2, 2024

@llvm/pr-subscribers-libcxx

Author: Mital Ashok (MitalAshok)

Changes

https://cplusplus.github.io/CWG/issues/2137.html

This change was previously made as part of 9247013 (#77768) and later reverted in 6e4930c

This change is still needed because the comment is still true: A standards-conformant compiler is currently supposed to fail this test.

This also means that any future work on CWG2137 with Clang would not need to modify the libc++ test suite


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

1 Files Affected:

  • (modified) libcxx/test/std/utilities/utility/pairs/pairs.pair/ctor.pair_U_V_move.pass.cpp (+22-1)
diff --git a/libcxx/test/std/utilities/utility/pairs/pairs.pair/ctor.pair_U_V_move.pass.cpp b/libcxx/test/std/utilities/utility/pairs/pairs.pair/ctor.pair_U_V_move.pass.cpp
index 3b2d093eb34d4..8ba9b5696eff1 100644
--- a/libcxx/test/std/utilities/utility/pairs/pairs.pair/ctor.pair_U_V_move.pass.cpp
+++ b/libcxx/test/std/utilities/utility/pairs/pairs.pair/ctor.pair_U_V_move.pass.cpp
@@ -121,7 +121,28 @@ int main(int, char**)
         test_pair_rv<CopyOnly, CopyOnly&>();
         test_pair_rv<CopyOnly, CopyOnly&&>();
 
-        test_pair_rv<ExplicitTypes::CopyOnly, ExplicitTypes::CopyOnly>();
+        /* For ExplicitTypes::CopyOnly, two of the viable candidates for initializing from a non-const xvalue are:
+         *   pair(const pair&);  // (defaulted copy constructor)
+         *   template<class U1, class U2> explicit pair(const pair<U1, U2>&&); [U1 = ExplicitTypes::CopyOnly, U2 = int]
+         *
+         * This results in diverging behavior for test_convertible which uses copy-list-initialization.
+         * Prior to CWG2137, this would have selected the first (non-explicit) ctor as explicit ctors
+         * would not be considered. Afterwards, it should select the second since it is a better match,
+         * and then failed because it is explicit.
+         *
+         * This may change with future defect reports, and some compilers only have partial support
+         * for CWG2137, so use std::is_convertible directly to avoid a copy-list-initialization
+         */
+        {
+          using P1  = std::pair<ExplicitTypes::CopyOnly, int>;
+          using P2  = std::pair<int, ExplicitTypes::CopyOnly>;
+          using UP1 = std::pair<ExplicitTypes::CopyOnly, int>&&;
+          using UP2 = std::pair<int, ExplicitTypes::CopyOnly>&&;
+          static_assert(std::is_constructible<P1, UP1>::value, "");
+          static_assert(std::is_convertible<P1, UP1>::value, "");
+          static_assert(std::is_constructible<P2, UP2>::value, "");
+          static_assert(std::is_convertible<P2, UP2>::value, "");
+        }
         test_pair_rv<ExplicitTypes::CopyOnly, ExplicitTypes::CopyOnly&, true, false>();
         test_pair_rv<ExplicitTypes::CopyOnly, ExplicitTypes::CopyOnly&&, true, false>();
 

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.

Thanks for extracting. Ship it once CI is green.

@MitalAshok
Copy link
Contributor Author

@ldionne Should be good now

@ldionne ldionne merged commit 8426b51 into llvm:main Jul 6, 2024
56 checks passed
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