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

ldionne · 2024-06-10T15:09:38Z

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.

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.

llvmbot · 2024-06-10T15:10:13Z

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

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.

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

5 Files Affected:

(modified) libcxx/include/__memory/shared_ptr.h (+1-1)
(modified) libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/nullptr_t_deleter.pass.cpp (+2-3)
(modified) libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/nullptr_t_deleter_allocator.pass.cpp (+2-3)
(modified) libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter.pass.cpp (+6)
(modified) libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter_allocator.pass.cpp (+6)

diff --git a/libcxx/include/__memory/shared_ptr.h b/libcxx/include/__memory/shared_ptr.h
index 00db96185be7c..7b5002cb95d32 100644
--- a/libcxx/include/__memory/shared_ptr.h
+++ b/libcxx/include/__memory/shared_ptr.h
@@ -404,7 +404,7 @@ struct __shared_ptr_deleter_ctor_reqs {
 };
 
 template <class _Dp, class _Tp>
-using __shared_ptr_nullptr_deleter_ctor_reqs = _And<is_move_constructible<_Dp>, __well_formed_deleter<_Dp, nullptr_t> >;
+using __shared_ptr_nullptr_deleter_ctor_reqs = _And<is_move_constructible<_Dp>, __well_formed_deleter<_Dp, _Tp*> >;
 
 #if defined(_LIBCPP_ABI_ENABLE_SHARED_PTR_TRIVIAL_ABI)
 #  define _LIBCPP_SHARED_PTR_TRIVIAL_ABI __attribute__((__trivial_abi__))
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 13340ed5294c0..9c93f2e361d24 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
@@ -33,16 +33,15 @@ int A::count = 0;
 // 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, 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, test_deleter<int[5]> >::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, "");
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 53ca6fb5b234d..f95b9e68dad01 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
@@ -34,16 +34,15 @@ int A::count = 0;
 // 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, 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, test_deleter<int[5]>, 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, "");
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 562acf56d96fe..bcae00ef2e031 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
@@ -115,6 +115,12 @@ int main(int, char**)
     }
 #endif // TEST_STD_VER >= 11
 
+    {
+        // See https://github.com/llvm/llvm-project/pull/93071#issuecomment-2158494851
+        auto deleter = [](auto pointer) { delete pointer; };
+        std::shared_ptr<int> p(new int, deleter);
+    }
+
   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 9dffbcdd59a73..66adff62b0707 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
@@ -165,5 +165,11 @@ int main(int, char**)
                                              test_allocator<Derived[4]> >::value, "");
     }
 
+    {
+        // See https://github.com/llvm/llvm-project/pull/93071#issuecomment-2158494851
+        auto deleter = [](auto pointer) { delete pointer; };
+        std::shared_ptr<int> p(new int, deleter, std::allocator<int>());
+    }
+
   return 0;
 }

github-actions · 2024-06-10T15:12:24Z

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

You can test this locally with the following command:

git-clang-format --diff 6b21e170497ae69a387ff3eebae025926742bb84 5282c05d0e407718288f1b015c65ffd6621ff367 -- 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/pointer_deleter.pass.cpp b/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter.pass.cpp
index 97dd2fcb22..96814d5346 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
@@ -123,6 +123,6 @@ int main(int, char**)
     }
 #endif
 
-  test_function_type();
-  return 0;
+    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 b90c69efd9..b7d34da245 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
@@ -173,5 +173,5 @@ int main(int, char**)
     }
 #endif
 
-  return 0;
+    return 0;
 }

alexfh

LGTM. Thanks! This will allow avoiding unnecessary changes, given the expected resolution (and until there is resolution on the new LWG issue).

alexfh · 2024-06-11T20:36:06Z

Code formatter is still not happy.

alexfh · 2024-06-12T17:42:31Z

Apparently, people rely on std::shared_ptr<std::nullptr_t>, which stops working after this patch: https://gcc.godbolt.org/z/7zrqKT74v

#include <functional>
#include <memory>

void f(const std::function<void()>& deleter) {
    auto d = std::shared_ptr<std::nullptr_t>(
          nullptr, [deleter](std::nullptr_t) { deleter(); });
}

Do you know if std::shared_ptr<std::nullptr_t> should work by the standard?

alexfh · 2024-06-13T11:56:20Z

Oh, that's not all. Some code also relies on std::shared_ptr<void> to work, which also breaks with this commit: https://gcc.godbolt.org/z/xrT6rKxE5

    std::shared_ptr<void> v;
    v.reset(new float[10], std::default_delete<float[]>());

(found in https://github.com/gazebosim/gz-rendering/blob/f3d30738726d11d240907e30699ce4c66e8a0f50/src/ShaderParam.cc#L119, for example)

ldionne requested a review from a team as a code owner June 10, 2024 15:09

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

alexfh approved these changes Jun 10, 2024

View reviewed changes

ldionne added 2 commits June 11, 2024 10:18

Formatting

76cb903

Fix test in C++03/11

5282c05

ldionne merged commit 16f2aa1 into llvm:main Jun 11, 2024
53 of 57 checks passed

ldionne deleted the review/fix-shared_ptr-nullptr-constructor branch June 11, 2024 20:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ldionne commented Jun 10, 2024

llvmbot commented Jun 10, 2024

github-actions bot commented Jun 10, 2024 •

edited

Loading

alexfh left a comment

alexfh commented Jun 11, 2024

alexfh commented Jun 12, 2024

alexfh commented Jun 13, 2024 •

edited

Loading

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

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

Conversation

ldionne commented Jun 10, 2024

llvmbot commented Jun 10, 2024

github-actions bot commented Jun 10, 2024 • edited Loading

alexfh left a comment

Choose a reason for hiding this comment

alexfh commented Jun 11, 2024

alexfh commented Jun 12, 2024

alexfh commented Jun 13, 2024 • edited Loading

github-actions bot commented Jun 10, 2024 •

edited

Loading

alexfh commented Jun 13, 2024 •

edited

Loading