[libc++] Revert temporary attempt to implement LWG 4110 #95263
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) ChangesWhen 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. Full diff: https://github.com/llvm/llvm-project/pull/95263.diff 6 Files Affected:
diff --git a/libcxx/docs/Status/Cxx2cIssues.csv b/libcxx/docs/Status/Cxx2cIssues.csv
index 28359b7bb49ac..8d24457186310 100644
--- a/libcxx/docs/Status/Cxx2cIssues.csv
+++ b/libcxx/docs/Status/Cxx2cIssues.csv
@@ -65,5 +65,4 @@
"`3343 <https://wg21.link/LWG3343>`__","Ordering of calls to ``unlock()`` and ``notify_all()`` in Effects element of ``notify_all_at_thread_exit()`` should be reversed","Not Yet Adopted","|Complete|","16.0",""
"XXXX","","The sys_info range should be affected by save","Not Yet Adopted","|Complete|","19.0"
"`4071 <https://wg21.link/LWG4071>`__","","``reference_wrapper`` comparisons are not SFINAE-friendly","Not Yet Adopted","|Complete|","19.0"
-"`4110 <https://wg21.link/LWG4110>`__","","``shared_ptr(nullptr_t, Deleter)`` is overconstrained, breaking some sensible deleters","Not Yet Adopted","|Complete|","19.0"
"","","","","",""
diff --git a/libcxx/include/__memory/shared_ptr.h b/libcxx/include/__memory/shared_ptr.h
index 7b5002cb95d32..00db96185be7c 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, _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__))
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 4ea752b36bd01..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
@@ -32,16 +32,17 @@ 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, 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[5]> >::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, "");
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 a479b24c4595a..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
@@ -33,21 +33,17 @@ 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, 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[5]>, 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, "");
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 95dcb92b51993..562acf56d96fe 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,14 +115,6 @@ 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);
- }
-#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 89e7d0b02d421..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
@@ -165,13 +165,5 @@ int main(int, char**)
test_allocator<Derived[4]> >::value, "");
}
-#if TEST_STD_VER >= 14
- {
- // LWG 4110
- auto deleter = [](auto pointer) { delete pointer; };
- std::shared_ptr<int> p(new int, deleter, std::allocator<int>());
- }
-#endif
-
return 0;
}
|
You can test this locally with the following command:git-clang-format --diff a9883739571f0adbe33219a74a934be7f8bd4f62 ad868137f19ba4ac0ccb1c144a935e4ba8ce3d33 -- 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.cppView 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..81ad84f609 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
@@ -32,17 +32,17 @@ 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, 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> >::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 53ca6fb5b2..f95358c5cf 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
@@ -33,17 +33,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>, 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, 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>, 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, "");
|
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.
ldionne
force-pushed
the
review/revert-shared-ptr-lwg-issue
branch
from
2b6f494 to
ad86813
Compare
alexfh
approved these changes
Jun 14, 2024
There was a problem hiding this comment.
Yep, the cure is worse than the disease in this case :( The impact of the original change is manageable, while the fix breaks a lot more code. Thanks for trying though! IMHO, process-wise it would be nicer to hold LWG3233 implementation until LWG4110 resolution is agreed upon, but the suggested fix for user code after LWG3233 (using auto* for deleter parameters) is a good change anyways.
Looks good modulo pre-merge checks.
|
Our CI is kinda down the drain right now but I'm going to merge this to get back to a "cleaner" state. I basically should never have landed that patch. We can follow-up on #93071 for the next steps. |
huixie90
added a commit
to huixie90/llvm-project
that referenced
this pull request
Jun 19, 2024
…10 (llvm#95263)"" This reverts commit 8bdab5c.
huixie90
added a commit
to huixie90/llvm-project
that referenced
this pull request
Jun 22, 2024
…10 (llvm#95263)"" This reverts commit 8bdab5c.
huixie90
added a commit
that referenced
this pull request
Jun 26, 2024
…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
AlexisPerry
pushed a commit
to llvm-project-tlp/llvm-project
that referenced
this pull request
Jul 9, 2024
…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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.