Skip to content
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++] Simplify the implementation of LWG2762 (noexcept for unique_ptr) #102032

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Aug 5, 2024

I had originally made some comments in https://reviews.llvm.org/D128214 that were followed when we implemented LWG2762. I don't understand why I made these comments anymore, but either way it seems like I was wrong since using unique_ptr<void>::operator* should be ill-formed. All other implementations also make that ill-formed.

@ldionne ldionne requested a review from a team as a code owner August 5, 2024 18:19
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Aug 5, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 5, 2024

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

I had originally made some comments in https://reviews.llvm.org/D128214 that were followed when we implemented LWG2762. I don't understand why I made these comments anymore, but either way it seems like I was wrong since using unique_ptr&lt;void&gt;::operator* should be ill-formed. All other implementations also make that ill-formed.


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

3 Files Affected:

  • (modified) libcxx/include/__memory/unique_ptr.h (+1-13)
  • (removed) libcxx/test/std/utilities/memory/unique.ptr/noexcept_operator_star.compile.pass.cpp (-25)
  • (modified) libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/dereference.single.pass.cpp (-4)
diff --git a/libcxx/include/__memory/unique_ptr.h b/libcxx/include/__memory/unique_ptr.h
index f75259473efb1..7f5e0ea243c95 100644
--- a/libcxx/include/__memory/unique_ptr.h
+++ b/libcxx/include/__memory/unique_ptr.h
@@ -36,7 +36,6 @@
 #include <__type_traits/is_trivially_relocatable.h>
 #include <__type_traits/is_void.h>
 #include <__type_traits/remove_extent.h>
-#include <__type_traits/remove_pointer.h>
 #include <__type_traits/type_identity.h>
 #include <__utility/declval.h>
 #include <__utility/forward.h>
@@ -52,17 +51,6 @@ _LIBCPP_PUSH_MACROS
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
-#ifndef _LIBCPP_CXX03_LANG
-
-template <class _Ptr>
-struct __is_noexcept_deref_or_void {
-  static constexpr bool value = noexcept(*std::declval<_Ptr>());
-};
-
-template <>
-struct __is_noexcept_deref_or_void<void*> : true_type {};
-#endif
-
 template <class _Tp>
 struct _LIBCPP_TEMPLATE_VIS default_delete {
   static_assert(!is_function<_Tp>::value, "default_delete cannot be instantiated for function types");
@@ -266,7 +254,7 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr {
   }
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 __add_lvalue_reference_t<_Tp> operator*() const
-      _NOEXCEPT_(__is_noexcept_deref_or_void<pointer>::value) {
+      _NOEXCEPT_(_NOEXCEPT_(*std::declval<pointer>())) {
     return *__ptr_.first();
   }
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 pointer operator->() const _NOEXCEPT { return __ptr_.first(); }
diff --git a/libcxx/test/std/utilities/memory/unique.ptr/noexcept_operator_star.compile.pass.cpp b/libcxx/test/std/utilities/memory/unique.ptr/noexcept_operator_star.compile.pass.cpp
deleted file mode 100644
index a2d788005f0cd..0000000000000
--- a/libcxx/test/std/utilities/memory/unique.ptr/noexcept_operator_star.compile.pass.cpp
+++ /dev/null
@@ -1,25 +0,0 @@
-//===----------------------------------------------------------------------===//
-//
-// 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
-//
-//===----------------------------------------------------------------------===//
-
-// UNSUPPORTED: c++03
-
-// unique_ptr
-
-// add_lvalue_reference_t<T> operator*() const noexcept(noexcept(*declval<pointer>()));
-
-// Dereferencing pointer directly in noexcept fails for a void pointer.  This
-// is not SFINAE-ed away leading to a hard error. The issue was originally
-// triggered by
-// test/std/utilities/memory/unique.ptr/iterator_concept_conformance.compile.pass.cpp
-//
-// This test validates whether the code compiles.
-
-#include <memory>
-
-extern const std::unique_ptr<void> p;
-void f() { [[maybe_unused]] bool b = noexcept(p.operator*()); }
diff --git a/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/dereference.single.pass.cpp b/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/dereference.single.pass.cpp
index 17902a4ce27a4..ba7f8bbb76222 100644
--- a/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/dereference.single.pass.cpp
+++ b/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/dereference.single.pass.cpp
@@ -31,10 +31,6 @@ struct Deleter {
 #endif
 
 TEST_CONSTEXPR_CXX23 bool test() {
-  ASSERT_NOEXCEPT(*(std::unique_ptr<void>{}));
-#if TEST_STD_VER >= 11
-  static_assert(noexcept(*std::declval<std::unique_ptr<void>>()), "");
-#endif
   {
     std::unique_ptr<int> p(new int(3));
     assert(*p == 3);

@mordante
Copy link
Member

mordante commented Aug 5, 2024

IIRC there were issue to test static_assert(noexcept(unique_ptr<void>::operator*())); But let's see what the CI says.

// Dereferencing pointer directly in noexcept fails for a void pointer. This
// is not SFINAE-ed away leading to a hard error. The issue was originally
// triggered by
// test/std/utilities/memory/unique.ptr/iterator_concept_conformance.compile.pass.cpp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the cause for adding this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. FWIW this test passes after applying this patch locally. Perhaps it used to fail with an older compiler? I seem to recall older Clang instantiating noexcept too eagerly at some point in the past.

@ldionne
Copy link
Member Author

ldionne commented Aug 5, 2024

IIRC there were issue to test static_assert(noexcept(unique_ptr<void>::operator*())); But let's see what the CI says.

My understanding is that static_assert(noexcept(unique_ptr<void>::operator*())); is ill-formed. All other implementations produce a hard-error for that case, and that seems to be what the Standard specifies.

Copy link
Member

@mordante mordante left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the CI passing.

@frederick-vs-ja
Copy link
Contributor

It seems that the SFINAE-friendness of unique_ptr<void>::operator* was originally expected and tested in https://reviews.llvm.org/D100073 (57ebf3d). Should we just remove the test coverage?

@ldionne
Copy link
Member Author

ldionne commented Aug 13, 2024

It seems that the SFINAE-friendness of unique_ptr<void>::operator* was originally expected and tested in https://reviews.llvm.org/D100073 (57ebf3d). Should we just remove the test coverage?

Yes, I think that makes the most sense.

…ptr)

I had originally made some comments in https://reviews.llvm.org/D128214
that were followed when we implemented LWG2762. I don't understand why
I made these comments anymore, but either way it seems like I was wrong
since using `unique_ptr<void>::operator*` should be ill-formed. All other
implementations also make that ill-formed.
@ldionne ldionne force-pushed the review/unique_ptr_void_noexcept branch from 9e60765 to 7acf191 Compare August 13, 2024 13:55
@ldionne ldionne merged commit 29e51f8 into llvm:main Aug 13, 2024
55 checks passed
@ldionne ldionne deleted the review/unique_ptr_void_noexcept branch August 13, 2024 20:18
bwendling pushed a commit to bwendling/llvm-project that referenced this pull request Aug 15, 2024
…ptr) (llvm#102032)

I had originally made some comments in https://reviews.llvm.org/D128214
that were followed when we implemented LWG2762. I don't understand why I
made these comments anymore, but either way it seems like I was wrong
since using `unique_ptr<void>::operator*` should be ill-formed. All
other implementations also make that ill-formed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants