[libc++] LWG3233 Broken requirements for shared_ptr converting constructors #93071

huixie90 · 2024-05-22T17:17:03Z

No description provided.

…uctors

llvmbot · 2024-05-22T17:17:34Z

@llvm/pr-subscribers-libcxx

Author: Hui (huixie90)

Changes

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

7 Files Affected:

(modified) libcxx/docs/Status/Cxx20Issues.csv (+1-1)
(modified) libcxx/include/__memory/shared_ptr.h (+5-2)
(modified) libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/nullptr_t_deleter.pass.cpp (+20)
(modified) libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/nullptr_t_deleter_allocator.pass.cpp (+21)
(modified) libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter.pass.cpp (+4-32)
(modified) libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter_allocator.pass.cpp (+3-32)
(added) libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/types.h (+48)

diff --git a/libcxx/docs/Status/Cxx20Issues.csv b/libcxx/docs/Status/Cxx20Issues.csv
index db57b15256a62..fadbc5c6048f0 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","",""
 "`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 992b1ba43f100..de5707c4a67b0 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
@@ -498,7 +501,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 {
@@ -518,7 +521,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..6029f62c0219c 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 42225a4b0be7e..5720e6a3fe0bb 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, "");
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..1c12c533d5be5 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, "");
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..e01167241cd84
--- /dev/null
+++ b/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/types.h
@@ -0,0 +1,48 @@
+//===----------------------------------------------------------------------===//
+//
+// 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

github-actions · 2024-05-22T17:19:56Z

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

You can test this locally with the following command:

git-clang-format --diff 7d9634e527fe52bf20a9036be6e5771f8fc4de17 6822939af233eb1492be4c354deeb819f25bb721 -- libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/types.h libcxx/include/__memory/shared_ptr.h libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/nullptr_t_deleter.pass.cpp libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/nullptr_t_deleter_allocator.pass.cpp libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter.pass.cpp libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter_allocator.pass.cpp

View the diff from clang-format here.

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 13340ed529..fbc8e88251 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
@@ -31,21 +31,21 @@ 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, "");
+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[]>, 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, "");
+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 53ca6fb5b2..3eb042d29d 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
@@ -32,21 +32,35 @@ 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, "");
+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[]>, 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, "");
+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**)
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 9c1e9b72be..1bc9706199 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
@@ -40,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 main(int, char**)
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 9dffbcdd59..5a765dc5f9 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
@@ -39,12 +39,14 @@ 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[]>, 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[5]>, int (*)[5], test_deleter<int>, test_allocator<int> >::value, "");
 #endif

...t/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/types.h

…uctors (llvm#93071)

alexfh · 2024-06-08T23:24:47Z

This commit breaks the following code (https://gcc.godbolt.org/z/eoMMr6KjE):

    std::shared_ptr<int>(
      new int,
      [](auto pointer) { delete pointer; }
    );

Is this intended?

The code compiles fine with libstdc++ and with libc++ before this change.

The error after this commit is:

<source>:6:26: error: cannot delete expression of type 'std::nullptr_t'
    6 |       [](auto pointer) { delete pointer; }
      |                          ^      ~~~~~~~
/opt/compiler-explorer/clang-trunk-20240608/bin/../include/c++/v1/__memory/shared_ptr.h:391:50: note: in instantiation of function template specialization 'f()::(anonymous class)::operator()<std::nullptr_t>' requested here
  391 | template <class _Dp, class _Pt, class = decltype(std::declval<_Dp>()(std::declval<_Pt>()))>
      |                                                  ^
/opt/compiler-explorer/clang-trunk-20240608/bin/../include/c++/v1/__memory/shared_ptr.h:392:11: note: in instantiation of default argument for '__well_formed_deleter_test<(lambda at <source>:6:7), std::nullptr_t>' required here
  392 | true_type __well_formed_deleter_test(int);
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/opt/compiler-explorer/clang-trunk-20240608/bin/../include/c++/v1/__memory/shared_ptr.h:398:41: note: while substituting deduced template arguments into function template '__well_formed_deleter_test' [with _Dp = (lambda at <source>:6:7), _Pt = std::nullptr_t, $2 = (no value)]
  398 | struct __well_formed_deleter : decltype(std::__well_formed_deleter_test<_Dp, _Pt>(0)) {};
      |                                         ^
/opt/compiler-explorer/clang-trunk-20240608/bin/../include/c++/v1/__type_traits/conjunction.h:28:32: note: in instantiation of template class 'std::__well_formed_deleter<(lambda at <source>:6:7), std::nullptr_t>' requested here
   28 | __expand_to_true<__enable_if_t<_Pred::value>...> __and_helper(int);
      |                                ^
/opt/compiler-explorer/clang-trunk-20240608/bin/../include/c++/v1/__type_traits/conjunction.h:39:39: note: while substituting explicitly-specified template arguments into function template '__and_helper' 
   39 | using _And _LIBCPP_NODEBUG = decltype(std::__and_helper<_Pred...>(0));
      |                                       ^
/opt/compiler-explorer/clang-trunk-20240608/bin/../include/c++/v1/__memory/shared_ptr.h:407:1: note: in instantiation of template type alias '_And' requested here
  407 | using __shared_ptr_nullptr_deleter_ctor_reqs = _And<is_move_constructible<_Dp>, __well_formed_deleter<_Dp, nullptr_t> >;
      | ^
/opt/compiler-explorer/clang-trunk-20240608/bin/../include/c++/v1/__memory/shared_ptr.h:504:38: note: in instantiation of template type alias '__shared_ptr_nullptr_deleter_ctor_reqs' requested here
  504 |   template <class _Dp, __enable_if_t<__shared_ptr_nullptr_deleter_ctor_reqs<_Dp, _Tp>::value, int> = 0 >
      |                                      ^
/opt/compiler-explorer/clang-trunk-20240608/bin/../include/c++/v1/__memory/shared_ptr.h:505:25: note: while substituting prior template arguments into non-type template parameter [with _Dp = (lambda at <source>:6:7)]
  505 |   _LIBCPP_HIDE_FROM_ABI shared_ptr(nullptr_t __p, _Dp __d) : __ptr_(nullptr) {
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  506 | #ifndef _LIBCPP_HAS_NO_EXCEPTIONS
      | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  507 |     try {
      |     ~~~~~
  508 | #endif // _LIBCPP_HAS_NO_EXCEPTIONS
      | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  509 |       typedef typename __shared_ptr_default_allocator<_Tp>::type _AllocT;
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  510 |       typedef __shared_ptr_pointer<nullptr_t, _Dp, _AllocT> _CntrlBlk;
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  511 | #ifndef _LIBCPP_CXX03_LANG
      | ~~~~~~~~~~~~~~~~~~~~~~~~~~
  512 |       __cntrl_ = new _CntrlBlk(__p, std::move(__d), _AllocT());
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  513 | #else
      | ~~~~~
  514 |     __cntrl_ = new _CntrlBlk(__p, __d, _AllocT());
      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  515 | #endif // not _LIBCPP_CXX03_LANG
      | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  516 | #ifndef _LIBCPP_HAS_NO_EXCEPTIONS
      | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  517 |     } catch (...) {
      |     ~~~~~~~~~~~~~~~
  518 |       __d(__p);
      |       ~~~~~~~~~
  519 |       throw;
      |       ~~~~~~
  520 |     }
      |     ~
<source>:4:5: note: while substituting deduced template arguments into function template 'shared_ptr' [with _Dp = (lambda at <source>:6:7), $1 = (no value)]
    4 |     std::shared_ptr<int>(
      |     ^

ldionne · 2024-06-10T14:15:57Z

@alexfh I think this is correct behavior, however it is arguably not great. For example, MSVC behaves the same as libc++ after this patch: https://gcc.godbolt.org/z/xd5PdheeG

It looks like libstdc++ doesn't implement the resolution of this LWG issue yet and that's why they still accept this code.

Basically, what happens here is that the nullptr_t constructor overload of std::shared_ptr checks for the well-formedness of deleter(ptr), where ptr has type nullptr_t. This instantiates the lambda and hard-errors inside the body of the lambda, because that's not an immediate context for SFINAE.

@CaseyCarter Was this an intended effect of the wording changes in http://wg21.link/LWG3233 ? CC @jwakely also

Edit: I guess a better way of phrasing my question would be "Was the case of deleters defined as auto-taking lambdas considered when this LWG issue was resolved?"

jwakely · 2024-06-10T14:32:56Z

No, I don't think that was considered. I think for the two overloads taking nullptr_t the constraint should be that d((T*)nullptr) is well-formed, since that's what the implementation actually does.

I think this needs a new LWG issue.

jwakely · 2024-06-10T14:35:56Z

N.B. I don't think this was caused by LWG 3233, the old wording said "this constructor shall not participate in overload resolution unless is_move_constructible_v<D> is true, the expression d(p) is well-formed" so the broken constraint was always present. It was even there in C++11, and violating it led to undefined behaviour (rather than being ill-formed). Libc++ just never enforced it until now.

ldionne · 2024-06-10T14:49:20Z

Email sent to LWG chair.

jwakely · 2024-06-10T15:00:44Z

Got it, thanks!

This avoids breaking code that should arguably be valid but technically isn't after enforcing the constraints on shared_ptr's constructors. A new LWG issue was filed to fix this in the Standard. This patch applies the expected resolution of this issue to avoid flip-flopping users whose code should always be considered valid. See llvm#93071 for more context.

) This avoids breaking code that should arguably be valid but technically isn't after enforcing the constraints on shared_ptr's constructors. A new LWG issue was filed to fix this in the Standard. This patch applies the expected resolution of this issue to avoid flip-flopping users whose code should always be considered valid. See #93071 for more context.

jwakely · 2024-06-12T10:45:47Z

It was even there in C++11, and violating it led to undefined behaviour (rather than being ill-formed).

I'm wrong about this - it was fine in C++11, because we didn't try to test d(p) during overload resolution, so didn't get a hard error outside the immediate context. The problem is caused by checking the d(p) expression during overload resolution, when considering the shared_ptr(nullptr_t, D) ctor, which is never actually going to be selected for this call.

I think for the two overloads taking nullptr_t the constraint should be that d((T*)nullptr) is well-formed, since that's what the implementation actually does.

And I was wrong about this too! As pointed out by Peter Dimov. The implementation really does do d(nullptr) if the shared_ptr(nullptr_t, D) constructor is called. But the problematic example above doesn't actually call that constructor, it just evaluates its constraints during overload resolution.

ldionne · 2024-06-12T15:36:41Z

I read the discussion on the LWG reflector. I see the proposed resolution isn't actually correct.

Given this, and since it looks like this is going to require additional work both in WG21 and in the implementations, @alexfh I think I will have to revert my workaround for this, and you folks will have to change your code at least for the time being. I'm sorry if this ends up flip-flopping once WG21 solves this issue, but for the time being we have to be Standards conforming and we are standards conforming in the state where your code is broken. It's unfortunate but I think it's the only reasonable state to be in for libc++.

When I filed LWG4110 after the discussion in llvm#93071, I thought it was going to be a straightforward fix. It turns out that it isn't, so we should stay in the state where libc++ is Standards conforming even if that state leads to some reasonable code being rejected by the library. Once WG21 figures out what to do with this issue and votes on it, we'll implement it through our normal means. This reverts f638f7b and 16f2aa1.

jwakely · 2024-06-12T16:19:56Z

It was noted on the reflector that using [](auto* pointer) { delete pointer; } works fine, because that deleter gives a substitution error when the d(nullptr) constraint is tested, instead of a hard error outside the immediate context.

bgra8 · 2024-06-13T15:43:50Z

@ldionne this also stopped working: https://gcc.godbolt.org/z/K8ncoP4Pv, is that expected??

When I filed LWG4110 after the discussion in llvm#93071, I thought it was going to be a straightforward fix. It turns out that it isn't, so we should stay in the state where libc++ is Standards conforming even if that state leads to some reasonable code being rejected by the library. Once WG21 figures out what to do with this issue and votes on it, we'll implement it through our normal means. This reverts f638f7b and 16f2aa1.

When I filed LWG4110 after the discussion in #93071, I thought it was going to be a straightforward fix. It turns out that it isn't, so we should stay in the state where libc++ is Standards conforming even if that state leads to some reasonable code being rejected by the library. Once WG21 figures out what to do with this issue and votes on it, we'll implement it through our normal means. This reverts f638f7b and 16f2aa1.

ldionne · 2024-06-14T18:40:43Z

@ldionne this also stopped working: https://gcc.godbolt.org/z/K8ncoP4Pv, is that expected??

I don't know yet, I have not had time to look into it and I won't until next week.

For this reason, I am going to temporarily revert this patch since it seems to cause issues and we don't know yet whether we have a bug in there or not. It's also a small and self-contained patch so easy to revert/reapply. I will investigate this next week and we can re-apply it.

… constructors (#93071) This reverts commit d868f09, which was shown to break some code and we don't know yet whether the code should be valid or not. Reverting until we've had time to figure it out next week.

ldionne · 2024-06-17T14:31:33Z

We should be in a stable state right now, i.e. back to how things were before we tried fixing LWG3233. I just spoke to @huixie90 who agreed to pick this up again and look into whether https://gcc.godbolt.org/z/K8ncoP4Pv is valid code. We'll ping you when a new PR to fix this LWG issue is ready.

alexfh · 2024-06-17T15:33:02Z

Thank you @ldionne! We've done some cleanup in our code to prepare at least for relanding of LWG3233. I suppose, we shouldn't see any more fallout unless larger changes are made in this area.

But I think, stability of libc++ interface is valuable to other users as well. So figuring out how the end state should look like would be nice before doing the change.

…nverting constructors (llvm#93071)" This reverts commit e7d5635.

…ting constructors" (#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 (#93071)" - Revert "[libc++] Revert temporary attempt to implement LWG 4110 (#95263)" - test for default_delete - Revert "Revert "[libc++] Revert temporary attempt to implement LWG 4110 (#95263)"" - test for NULL

…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

[libc++] LWG3223 Broken requirements for shared_ptr converting constr…

eae0f4c

…uctors

huixie90 requested a review from a team as a code owner May 22, 2024 17:17

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label May 22, 2024

ldionne approved these changes May 24, 2024

View reviewed changes

...t/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/types.h Outdated Show resolved Hide resolved

address comment

6822939

huixie90 merged commit d868f09 into llvm:main May 29, 2024
53 of 54 checks passed

vg0204 pushed a commit to vg0204/llvm-project that referenced this pull request May 29, 2024

[libc++] LWG3223 Broken requirements for shared_ptr converting constr…

4406485

…uctors (llvm#93071)

ldionne mentioned this pull request Jun 10, 2024

[libc++] Tweak how we check constraints on shared_ptr(nullptr_t) #94996

Merged

ldionne mentioned this pull request Jun 12, 2024

[libc++] Revert temporary attempt to implement LWG 4110 #95263

Merged

var-const changed the title ~~[libc++] LWG3223 Broken requirements for shared_ptr converting constructors~~ [libc++] LWG3233 Broken requirements for shared_ptr converting constructors Jun 12, 2024

jpienaar mentioned this pull request Jun 17, 2024

[mlirc] Add missing extern C #95829

Merged

huixie90 added a commit to huixie90/llvm-project that referenced this pull request Jun 19, 2024

Revert "[libc++] Revert LWG3223 Broken requirements for shared_ptr co…

2b2be73

…nverting constructors (llvm#93071)" This reverts commit e7d5635.

huixie90 mentioned this pull request Jun 19, 2024

"[libc++] Try again LWG3233 Broken requirements for shared_ptr converting constructors" #96103

Merged

huixie90 added a commit to huixie90/llvm-project that referenced this pull request Jun 22, 2024

Revert "[libc++] Revert LWG3223 Broken requirements for shared_ptr co…

aca9c1a

…nverting constructors (llvm#93071)" This reverts commit e7d5635.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc++] LWG3233 Broken requirements for shared_ptr converting constructors #93071

[libc++] LWG3233 Broken requirements for shared_ptr converting constructors #93071

huixie90 commented May 22, 2024

llvmbot commented May 22, 2024

github-actions bot commented May 22, 2024 •

edited

Loading

alexfh commented Jun 8, 2024 •

edited

Loading

ldionne commented Jun 10, 2024 •

edited

Loading

jwakely commented Jun 10, 2024

jwakely commented Jun 10, 2024 •

edited

Loading

ldionne commented Jun 10, 2024

jwakely commented Jun 10, 2024

jwakely commented Jun 12, 2024

ldionne commented Jun 12, 2024

jwakely commented Jun 12, 2024

bgra8 commented Jun 13, 2024

ldionne commented Jun 14, 2024

ldionne commented Jun 17, 2024

alexfh commented Jun 17, 2024

[libc++] LWG3233 Broken requirements for shared_ptr converting constructors #93071

[libc++] LWG3233 Broken requirements for shared_ptr converting constructors #93071

Conversation

huixie90 commented May 22, 2024

llvmbot commented May 22, 2024

github-actions bot commented May 22, 2024 • edited Loading

alexfh commented Jun 8, 2024 • edited Loading

ldionne commented Jun 10, 2024 • edited Loading

jwakely commented Jun 10, 2024

jwakely commented Jun 10, 2024 • edited Loading

ldionne commented Jun 10, 2024

jwakely commented Jun 10, 2024

jwakely commented Jun 12, 2024

ldionne commented Jun 12, 2024

jwakely commented Jun 12, 2024

bgra8 commented Jun 13, 2024

ldionne commented Jun 14, 2024

ldionne commented Jun 17, 2024

alexfh commented Jun 17, 2024

github-actions bot commented May 22, 2024 •

edited

Loading

alexfh commented Jun 8, 2024 •

edited

Loading

ldionne commented Jun 10, 2024 •

edited

Loading

jwakely commented Jun 10, 2024 •

edited

Loading