Fixed shared_ptr comparisons with nullptr_t when spaceship is unavailable. #76781

Bekenn · 2024-01-03T05:38:46Z

This was causing compilation errors when attempting to compare a shared_ptr<T[]> with nullptr, as get() returns T* rather than T (*)[]. unique_ptr did not have this issue, but I've added tests to make sure.

github-actions · 2024-01-03T05:39:01Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-01-03T05:39:33Z

@llvm/pr-subscribers-libcxx

Author: James Touton (Bekenn)

Changes

@ldionne

This was causing compilation errors when attempting to compare a shared_ptr<T[]> with nullptr, as get() returns T* rather than T (*)[]. unique_ptr did not have this issue, but I've added tests to make sure.

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

3 Files Affected:

(modified) libcxx/include/__memory/shared_ptr.h (+2-2)
(modified) libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cmp/cmp_nullptr.pass.cpp (+31)
(modified) libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.special/cmp_nullptr.pass.cpp (+34)

diff --git a/libcxx/include/__memory/shared_ptr.h b/libcxx/include/__memory/shared_ptr.h
index 9aa938b2203121..9a73d439306d9e 100644
--- a/libcxx/include/__memory/shared_ptr.h
+++ b/libcxx/include/__memory/shared_ptr.h
@@ -1166,12 +1166,12 @@ inline _LIBCPP_HIDE_FROM_ABI bool operator!=(nullptr_t, const shared_ptr<_Tp>& _
 
 template <class _Tp>
 inline _LIBCPP_HIDE_FROM_ABI bool operator<(const shared_ptr<_Tp>& __x, nullptr_t) _NOEXCEPT {
-  return less<_Tp*>()(__x.get(), nullptr);
+  return less<typename shared_ptr<_Tp>::element_type*>()(__x.get(), nullptr);
 }
 
 template <class _Tp>
 inline _LIBCPP_HIDE_FROM_ABI bool operator<(nullptr_t, const shared_ptr<_Tp>& __x) _NOEXCEPT {
-  return less<_Tp*>()(nullptr, __x.get());
+  return less<typename shared_ptr<_Tp>::element_type*>()(nullptr, __x.get());
 }
 
 template <class _Tp>
diff --git a/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cmp/cmp_nullptr.pass.cpp b/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cmp/cmp_nullptr.pass.cpp
index 3bc8ef3799ab66..5a131714fce4c4 100644
--- a/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cmp/cmp_nullptr.pass.cpp
+++ b/libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cmp/cmp_nullptr.pass.cpp
@@ -87,5 +87,36 @@ int main(int, char**)
   assert((nullptr <=> p2) == std::strong_ordering::equivalent);
 #endif
 
+  const std::shared_ptr<int[]> p3(new int[1]);
+  assert(!(p3 == nullptr));
+  assert(!(nullptr == p3));
+  assert(!(p3 < nullptr));
+  assert((nullptr < p3));
+  assert(!(p3 <= nullptr));
+  assert((nullptr <= p3));
+  assert((p3 > nullptr));
+  assert(!(nullptr > p3));
+  assert((p3 >= nullptr));
+  assert(!(nullptr >= p3));
+#if TEST_STD_VER > 17
+  assert((nullptr <=> p3) == std::strong_ordering::less);
+  assert((p3 <=> nullptr) == std::strong_ordering::greater);
+#endif
+
+  const std::shared_ptr<int[]> p4;
+  assert((p4 == nullptr));
+  assert((nullptr == p4));
+  assert(!(p4 < nullptr));
+  assert(!(nullptr < p4));
+  assert((p4 <= nullptr));
+  assert((nullptr <= p4));
+  assert(!(p4 > nullptr));
+  assert(!(nullptr > p4));
+  assert((p4 >= nullptr));
+  assert((nullptr >= p4));
+#if TEST_STD_VER > 17
+  assert((nullptr <=> p4) == std::strong_ordering::equivalent);
+#endif
+
   return 0;
 }
diff --git a/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.special/cmp_nullptr.pass.cpp b/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.special/cmp_nullptr.pass.cpp
index ddd02a455c5883..9ee88e22884488 100644
--- a/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.special/cmp_nullptr.pass.cpp
+++ b/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.special/cmp_nullptr.pass.cpp
@@ -92,6 +92,40 @@ TEST_CONSTEXPR_CXX23 bool test() {
   assert((nullptr <=> p2) == std::strong_ordering::equivalent);
 #endif
 
+  const std::unique_ptr<int[]> p3(new int[1]);
+  assert(!(p3 == nullptr));
+  assert(!(nullptr == p3));
+  // A pointer to allocated storage and a nullptr can't be compared at compile-time
+  if (!TEST_IS_CONSTANT_EVALUATED) {
+    assert(!(p3 < nullptr));
+    assert((nullptr < p3));
+    assert(!(p3 <= nullptr));
+    assert((nullptr <= p3));
+    assert((p3 > nullptr));
+    assert(!(nullptr > p3));
+    assert((p3 >= nullptr));
+    assert(!(nullptr >= p3));
+#if TEST_STD_VER > 17
+    assert((nullptr <=> p3) == std::strong_ordering::less);
+    assert((p3 <=> nullptr) == std::strong_ordering::greater);
+#endif
+  }
+
+  const std::unique_ptr<int[]> p4;
+  assert((p4 == nullptr));
+  assert((nullptr == p4));
+  assert(!(p4 < nullptr));
+  assert(!(nullptr < p4));
+  assert((p4 <= nullptr));
+  assert((nullptr <= p4));
+  assert(!(p4 > nullptr));
+  assert(!(nullptr > p4));
+  assert((p4 >= nullptr));
+  assert((nullptr >= p4));
+#if TEST_STD_VER > 17
+  assert((nullptr <=> p4) == std::strong_ordering::equivalent);
+#endif
+
   return true;
 }

github-actions · 2024-01-03T08:24:43Z

✅ With the latest revision this PR passed the C/C++ code formatter.

mordante

Thanks for your patch!

A few minor comments.

mordante · 2024-01-07T13:27:56Z

...ties/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cmp/cmp_nullptr.pass.cpp

@@ -87,5 +87,38 @@ int main(int, char**)
  assert((nullptr <=> p2) == std::strong_ordering::equivalent);
 #endif

+#if TEST_STD_VER > 14


nowadays we typically use this style.

Suggested change

#if TEST_STD_VER > 14

#if TEST_STD_VER >= 17

Please apply to similar cases.

...ties/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.cmp/cmp_nullptr.pass.cpp

mordante

The code LGTM. However the CI fails, I expect a rebase will fix the CI.
After rebasing I'll approve the CI run that need approval.

…able.

mordante

Thanks! LGTM! Do you need help landing this patch?

Bekenn · 2024-01-09T19:39:00Z

I don't currently have write access, though I have contributed elsewhere in llvm-project before, and have some other stuff I'm working on.

…able. (llvm#76781) This was causing compilation errors when attempting to compare a `shared_ptr<T[]>` with `nullptr`, as `get()` returns `T*` rather than `T (*)[]`. `unique_ptr` did not have this issue, but I've added tests to make sure.

Bekenn requested a review from a team as a code owner January 3, 2024 05:38

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

Bekenn force-pushed the sharedptr-nullptr-cmp-fix branch 2 times, most recently from d673084 to 315a90f Compare January 3, 2024 09:02

mordante requested changes Jan 7, 2024

View reviewed changes

mordante self-assigned this Jan 7, 2024

mordante requested changes Jan 8, 2024

View reviewed changes

Bekenn added 3 commits January 8, 2024 10:13

Fixed shared_ptr comparisons with nullptr_t when spaceship is unavail…

0ee1d8f

…able.

Don't test shared_ptr<T[]> in language modes earlier than C++17.

5e6e73f

PR feedback

66744b0

Bekenn force-pushed the sharedptr-nullptr-cmp-fix branch from d3c9062 to 66744b0 Compare January 8, 2024 18:16

Bekenn requested a review from mordante January 9, 2024 05:35

mordante approved these changes Jan 9, 2024

View reviewed changes

mordante merged commit 65a1efc into llvm:main Jan 9, 2024

Bekenn deleted the sharedptr-nullptr-cmp-fix branch January 9, 2024 19:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixed shared_ptr comparisons with nullptr_t when spaceship is unavailable. #76781

Fixed shared_ptr comparisons with nullptr_t when spaceship is unavailable. #76781

Uh oh!

Bekenn commented Jan 3, 2024

Uh oh!

github-actions bot commented Jan 3, 2024

Uh oh!

llvmbot commented Jan 3, 2024

Uh oh!

github-actions bot commented Jan 3, 2024 •

edited

Loading

Uh oh!

mordante left a comment

Uh oh!

mordante Jan 7, 2024

Uh oh!

Bekenn Jan 7, 2024

Uh oh!

Uh oh!

mordante left a comment

Uh oh!

mordante left a comment

Uh oh!

Bekenn commented Jan 9, 2024

Uh oh!

Uh oh!

Fixed shared_ptr comparisons with nullptr_t when spaceship is unavailable. #76781

Fixed shared_ptr comparisons with nullptr_t when spaceship is unavailable. #76781

Uh oh!

Conversation

Bekenn commented Jan 3, 2024

Uh oh!

github-actions bot commented Jan 3, 2024

Uh oh!

llvmbot commented Jan 3, 2024

Uh oh!

github-actions bot commented Jan 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mordante left a comment

Choose a reason for hiding this comment

Uh oh!

mordante Jan 7, 2024

Choose a reason for hiding this comment

Uh oh!

Bekenn Jan 7, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mordante left a comment

Choose a reason for hiding this comment

Uh oh!

mordante left a comment

Choose a reason for hiding this comment

Uh oh!

Bekenn commented Jan 9, 2024

Uh oh!

Uh oh!

github-actions bot commented Jan 3, 2024 •

edited

Loading