-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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++] Try again LWG3233 Broken requirements for shared_ptr converting constructors" #96103
Conversation
@llvm/pr-subscribers-libcxx Author: Hui (huixie90) Changes
Full diff: https://github.com/llvm/llvm-project/pull/96103.diff 7 Files Affected:
diff --git a/libcxx/docs/Status/Cxx20Issues.csv b/libcxx/docs/Status/Cxx20Issues.csv
index 6453f227cfcc2..e748ff6ad749b 100644
--- a/libcxx/docs/Status/Cxx20Issues.csv
+++ b/libcxx/docs/Status/Cxx20Issues.csv
@@ -200,7 +200,7 @@
"`3200 <https://wg21.link/LWG3200>`__","``midpoint``\ should not constrain ``T``\ is complete","Prague","|Nothing To Do|",""
"`3201 <https://wg21.link/LWG3201>`__","``lerp``\ should be marked as ``noexcept``\ ","Prague","|Complete|",""
"`3226 <https://wg21.link/LWG3226>`__","``zoned_time``\ constructor from ``string_view``\ should accept ``zoned_time<Duration2, TimeZonePtr2>``\ ","Prague","","","|chrono|"
-"`3233 <https://wg21.link/LWG3233>`__","Broken requirements for ``shared_ptr``\ converting constructors","Prague","",""
+"`3233 <https://wg21.link/LWG3233>`__","Broken requirements for ``shared_ptr``\ converting constructors","Prague","|Complete|","19.0"
"`3237 <https://wg21.link/LWG3237>`__","LWG 3038 and 3190 have inconsistent PRs","Prague","|Complete|","16.0"
"`3238 <https://wg21.link/LWG3238>`__","Insufficiently-defined behavior of ``std::function``\ deduction guides","Prague","|Nothing To Do|",""
"`3242 <https://wg21.link/LWG3242>`__","``std::format``\ : missing rules for ``arg-id``\ in ``width``\ and ``precision``\ ","Prague","|Complete|","14.0","|format|"
diff --git a/libcxx/include/__memory/shared_ptr.h b/libcxx/include/__memory/shared_ptr.h
index 358a851958db1..c4001a2bc927d 100644
--- a/libcxx/include/__memory/shared_ptr.h
+++ b/libcxx/include/__memory/shared_ptr.h
@@ -403,6 +403,9 @@ struct __shared_ptr_deleter_ctor_reqs {
__well_formed_deleter<_Dp, _Yp*>::value;
};
+template <class _Dp, class _Tp>
+using __shared_ptr_nullptr_deleter_ctor_reqs = _And<is_move_constructible<_Dp>, __well_formed_deleter<_Dp, nullptr_t> >;
+
#if defined(_LIBCPP_ABI_ENABLE_SHARED_PTR_TRIVIAL_ABI)
# define _LIBCPP_SHARED_PTR_TRIVIAL_ABI __attribute__((__trivial_abi__))
#else
@@ -502,7 +505,7 @@ class _LIBCPP_SHARED_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS shared_ptr {
#endif // _LIBCPP_HAS_NO_EXCEPTIONS
}
- template <class _Dp>
+ template <class _Dp, __enable_if_t<__shared_ptr_nullptr_deleter_ctor_reqs<_Dp, _Tp>::value, int> = 0 >
_LIBCPP_HIDE_FROM_ABI shared_ptr(nullptr_t __p, _Dp __d) : __ptr_(nullptr) {
#ifndef _LIBCPP_HAS_NO_EXCEPTIONS
try {
@@ -522,7 +525,7 @@ class _LIBCPP_SHARED_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS shared_ptr {
#endif // _LIBCPP_HAS_NO_EXCEPTIONS
}
- template <class _Dp, class _Alloc>
+ template <class _Dp, class _Alloc, __enable_if_t<__shared_ptr_nullptr_deleter_ctor_reqs<_Dp, _Tp>::value, int> = 0 >
_LIBCPP_HIDE_FROM_ABI shared_ptr(nullptr_t __p, _Dp __d, _Alloc __a) : __ptr_(nullptr) {
#ifndef _LIBCPP_HAS_NO_EXCEPTIONS
try {
diff --git a/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/nullptr_t_deleter.pass.cpp b/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/nullptr_t_deleter.pass.cpp
index 49497b6956b9f..13340ed5294c0 100644
--- a/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/nullptr_t_deleter.pass.cpp
+++ b/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/nullptr_t_deleter.pass.cpp
@@ -17,6 +17,7 @@
#include "test_macros.h"
#include "deleter_types.h"
+#include "types.h"
struct A
{
static int count;
@@ -28,6 +29,25 @@ struct A
int A::count = 0;
+// LWG 3233. Broken requirements for shared_ptr converting constructors
+// https://cplusplus.github.io/LWG/issue3233
+static_assert( std::is_constructible<std::shared_ptr<int>, std::nullptr_t, test_deleter<int> >::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int>, std::nullptr_t, bad_deleter>::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int>, std::nullptr_t, no_nullptr_deleter>::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int>, std::nullptr_t, no_move_deleter>::value, "");
+
+#if TEST_STD_VER >= 17
+static_assert( std::is_constructible<std::shared_ptr<int[]>, std::nullptr_t, test_deleter<int> >::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int[]>, std::nullptr_t, bad_deleter>::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int[]>, std::nullptr_t, no_nullptr_deleter>::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int[]>, std::nullptr_t, no_move_deleter>::value, "");
+
+static_assert( std::is_constructible<std::shared_ptr<int[5]>, std::nullptr_t, test_deleter<int> >::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int[5]>, std::nullptr_t, bad_deleter>::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int[5]>, std::nullptr_t, no_nullptr_deleter>::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int[5]>, std::nullptr_t, no_move_deleter>::value, "");
+#endif
+
int main(int, char**)
{
{
diff --git a/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/nullptr_t_deleter_allocator.pass.cpp b/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/nullptr_t_deleter_allocator.pass.cpp
index 4e9fc227b99e8..53ca6fb5b234d 100644
--- a/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/nullptr_t_deleter_allocator.pass.cpp
+++ b/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/nullptr_t_deleter_allocator.pass.cpp
@@ -17,6 +17,8 @@
#include "test_allocator.h"
#include "min_allocator.h"
+#include "types.h"
+
struct A
{
static int count;
@@ -28,6 +30,25 @@ struct A
int A::count = 0;
+// LWG 3233. Broken requirements for shared_ptr converting constructors
+// https://cplusplus.github.io/LWG/issue3233
+static_assert( std::is_constructible<std::shared_ptr<int>, std::nullptr_t, test_deleter<int>, test_allocator<int> >::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int>, std::nullptr_t, bad_deleter, test_allocator<int> >::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int>, std::nullptr_t, no_nullptr_deleter, test_allocator<int> >::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int>, std::nullptr_t, no_move_deleter, test_allocator<int> >::value, "");
+
+#if TEST_STD_VER >= 17
+static_assert( std::is_constructible<std::shared_ptr<int[]>, std::nullptr_t, test_deleter<int>, test_allocator<int> >::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int[]>, std::nullptr_t, bad_deleter, test_allocator<int> >::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int[]>, std::nullptr_t, no_nullptr_deleter, test_allocator<int> >::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int[]>, std::nullptr_t, no_move_deleter, test_allocator<int> >::value, "");
+
+static_assert( std::is_constructible<std::shared_ptr<int[5]>, std::nullptr_t, test_deleter<int>, test_allocator<int> >::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int[5]>, std::nullptr_t, bad_deleter, test_allocator<int> >::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int[5]>, std::nullptr_t, no_nullptr_deleter, test_allocator<int> >::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int[5]>, std::nullptr_t, no_move_deleter, test_allocator<int> >::value, "");
+#endif
+
int main(int, char**)
{
test_allocator_statistics alloc_stats;
diff --git a/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter.pass.cpp b/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter.pass.cpp
index 0f4aa0f5c0689..017e012e34b10 100644
--- a/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter.pass.cpp
+++ b/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter.pass.cpp
@@ -17,6 +17,8 @@
#include "test_macros.h"
#include "deleter_types.h"
+#include "types.h"
+
struct A
{
static int count;
@@ -28,38 +30,8 @@ struct A
int A::count = 0;
-struct bad_ty { };
-
-struct bad_deleter
-{
- void operator()(bad_ty) { }
-};
-
-struct no_move_deleter
-{
- no_move_deleter(no_move_deleter const&) = delete;
- no_move_deleter(no_move_deleter &&) = delete;
- void operator()(int*) { }
-};
-
-static_assert(!std::is_move_constructible<no_move_deleter>::value, "");
-
-struct Base { };
-struct Derived : Base { };
-
-template<class T>
-class MoveDeleter
-{
- MoveDeleter();
- MoveDeleter(MoveDeleter const&);
-public:
- MoveDeleter(MoveDeleter&&) {}
-
- explicit MoveDeleter(int) {}
-
- void operator()(T* ptr) { delete ptr; }
-};
-
+// LWG 3233. Broken requirements for shared_ptr converting constructors
+// https://cplusplus.github.io/LWG/issue3233
// https://llvm.org/PR60258
// Invalid constructor SFINAE for std::shared_ptr's array ctors
static_assert( std::is_constructible<std::shared_ptr<int>, int*, test_deleter<int> >::value, "");
@@ -68,12 +40,12 @@ static_assert( std::is_constructible<std::shared_ptr<Base>, Derived*, test_dele
static_assert(!std::is_constructible<std::shared_ptr<A>, int*, test_deleter<A> >::value, "");
#if TEST_STD_VER >= 17
-static_assert( std::is_constructible<std::shared_ptr<int[]>, int*, test_deleter<int>>::value, "");
+static_assert( std::is_constructible<std::shared_ptr<int[]>, int*, test_deleter<int> >::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int[]>, int*, bad_deleter>::value, "");
-static_assert(!std::is_constructible<std::shared_ptr<int[]>, int(*)[], test_deleter<int>>::value, "");
-static_assert( std::is_constructible<std::shared_ptr<int[5]>, int*, test_deleter<int>>::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int[]>, int(*)[], test_deleter<int> >::value, "");
+static_assert( std::is_constructible<std::shared_ptr<int[5]>, int*, test_deleter<int> >::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int[5]>, int*, bad_deleter>::value, "");
-static_assert(!std::is_constructible<std::shared_ptr<int[5]>, int(*)[5], test_deleter<int>>::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int[5]>, int(*)[5], test_deleter<int> >::value, "");
#endif
int f() { return 5; }
@@ -143,6 +115,25 @@ int main(int, char**)
}
#endif // TEST_STD_VER >= 11
+#if TEST_STD_VER >= 14
+ {
+ // LWG 4110
+ auto deleter = [](auto pointer) { delete pointer; };
+ std::shared_ptr<int> p(new int, deleter);
+ }
+
+ {
+ std::shared_ptr<int> p(NULL, [](auto){});
+ }
+#endif
+
+#if TEST_STD_VER >= 17
+ {
+ // See https://github.com/llvm/llvm-project/pull/93071#issuecomment-2166047398
+ std::shared_ptr<char[]> a(new char[10], std::default_delete<char[]>());
+ }
+#endif
+
test_function_type();
return 0;
}
diff --git a/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter_allocator.pass.cpp b/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter_allocator.pass.cpp
index a110525b9b922..9dffbcdd59a73 100644
--- a/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter_allocator.pass.cpp
+++ b/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter_allocator.pass.cpp
@@ -17,6 +17,7 @@
#include "test_allocator.h"
#include "min_allocator.h"
+#include "types.h"
struct A
{
static int count;
@@ -28,38 +29,8 @@ struct A
int A::count = 0;
-struct bad_ty { };
-
-struct bad_deleter
-{
- void operator()(bad_ty) { }
-};
-
-struct no_move_deleter
-{
- no_move_deleter(no_move_deleter const&) = delete;
- no_move_deleter(no_move_deleter &&) = delete;
- void operator()(int*) { }
-};
-
-static_assert(!std::is_move_constructible<no_move_deleter>::value, "");
-
-struct Base { };
-struct Derived : Base { };
-
-template<class T>
-class MoveDeleter
-{
- MoveDeleter();
- MoveDeleter(MoveDeleter const&);
-public:
- MoveDeleter(MoveDeleter&&) {}
-
- explicit MoveDeleter(int) {}
-
- void operator()(T* ptr) { delete ptr; }
-};
-
+// LWG 3233. Broken requirements for shared_ptr converting constructors
+// https://cplusplus.github.io/LWG/issue3233
// https://llvm.org/PR60258
// Invalid constructor SFINAE for std::shared_ptr's array ctors
static_assert( std::is_constructible<std::shared_ptr<int>, int*, test_deleter<int>, test_allocator<int> >::value, "");
@@ -68,12 +39,12 @@ static_assert( std::is_constructible<std::shared_ptr<Base>, Derived*, test_dele
static_assert(!std::is_constructible<std::shared_ptr<A>, int*, test_deleter<A>, test_allocator<A> >::value, "");
#if TEST_STD_VER >= 17
-static_assert( std::is_constructible<std::shared_ptr<int[]>, int*, test_deleter<int>, test_allocator<int>>::value, "");
-static_assert(!std::is_constructible<std::shared_ptr<int[]>, int*, bad_deleter, test_allocator<int>>::value, "");
-static_assert(!std::is_constructible<std::shared_ptr<int[]>, int(*)[], test_deleter<int>, test_allocator<int>>::value, "");
-static_assert( std::is_constructible<std::shared_ptr<int[5]>, int*, test_deleter<int>, test_allocator<int>>::value, "");
-static_assert(!std::is_constructible<std::shared_ptr<int[5]>, int*, bad_deleter, test_allocator<int>>::value, "");
-static_assert(!std::is_constructible<std::shared_ptr<int[5]>, int(*)[5], test_deleter<int>, test_allocator<int>>::value, "");
+static_assert( std::is_constructible<std::shared_ptr<int[]>, int*, test_deleter<int>, test_allocator<int> >::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int[]>, int*, bad_deleter, test_allocator<int> >::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int[]>, int(*)[], test_deleter<int>, test_allocator<int> >::value, "");
+static_assert( std::is_constructible<std::shared_ptr<int[5]>, int*, test_deleter<int>, test_allocator<int> >::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int[5]>, int*, bad_deleter, test_allocator<int> >::value, "");
+static_assert(!std::is_constructible<std::shared_ptr<int[5]>, int(*)[5], test_deleter<int>, test_allocator<int> >::value, "");
#endif
diff --git a/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/types.h b/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/types.h
new file mode 100644
index 0000000000000..5bfb3d70febea
--- /dev/null
+++ b/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/types.h
@@ -0,0 +1,49 @@
+//===----------------------------------------------------------------------===//
+//
+// 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 TEST_STD_UTILITIES_MEMORY_UTIL_SMARTPTR_SHARED_CONST_TYPES_H
+#define TEST_STD_UTILITIES_MEMORY_UTIL_SMARTPTR_SHARED_CONST_TYPES_H
+
+#include <type_traits>
+
+struct bad_ty {};
+
+struct bad_deleter {
+ void operator()(bad_ty) {}
+};
+
+struct no_move_deleter {
+ no_move_deleter(no_move_deleter const&) = delete;
+ no_move_deleter(no_move_deleter&&) = delete;
+ void operator()(int*) {}
+};
+
+static_assert(!std::is_move_constructible<no_move_deleter>::value, "");
+
+struct no_nullptr_deleter {
+ void operator()(int*) const {}
+ void operator()(std::nullptr_t) const = delete;
+};
+
+struct Base {};
+struct Derived : Base {};
+
+template <class T>
+class MoveDeleter {
+ MoveDeleter();
+ MoveDeleter(MoveDeleter const&);
+
+public:
+ MoveDeleter(MoveDeleter&&) {}
+
+ explicit MoveDeleter(int) {}
+
+ void operator()(T* ptr) { delete ptr; }
+};
+
+#endif // TEST_STD_UTILITIES_MEMORY_UTIL_SMARTPTR_SHARED_CONST_TYPES_H
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM w/ green CI and nit.
I don't really understand why this fixes the issue we were seeing but it does, so that's good enough until the LWG issue is sorted out.
...t/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/types.h
Outdated
Show resolved
Hide resolved
…nverting constructors (llvm#93071)" This reverts commit e7d5635.
…10 (llvm#95263)"" This reverts commit 8bdab5c.
0b690f5
to
e0eacc5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, merge whenever you are ready.
…ting constructors" (llvm#96103) Try it again. Use the approach suggested by Tim in the LWG thread : using function default argument SFINAE - Revert "[libc++] Revert LWG3233 Broken requirements for shared_ptr converting constructors (llvm#93071)" - Revert "[libc++] Revert temporary attempt to implement LWG 4110 (llvm#95263)" - test for default_delete - Revert "Revert "[libc++] Revert temporary attempt to implement LWG 4110 (llvm#95263)"" - test for NULL
Try it again. Use the approach suggested by Tim in the LWG thread : using function default argument SFINAE