[libcxx-commits] [PATCH] D128214: [libc++] Makes `unique_ptr operator*() noexcept.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 3 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/include/__memory/unique_ptr.h:272-286
+  _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX23 __add_lvalue_reference_t<_Tp> operator*() const
+#if _LIBCPP_STD_VER >= 20
+      noexcept([] {
+        if constexpr (is_void_v<remove_pointer_t<pointer>>)
+          return true;
+        else
+          // This avoids evaluating noexcept(*declval<pointer>() when pointer == void*.
----------------
ldionne wrote:
> I think I understand the issue now. So what's going on is that the contents of `noexcept` are not SFINAE-d on, and so this becomes a hard error. A test case we should add (in all standard modes) is this (https://godbolt.org/z/Teforbb6j):
> 
> ```
> #include <type_traits>
> #include <memory>
> 
> 
> template <class T, class = void>
> struct has_dereference : std::false_type { };
> 
> template <class T>
> struct has_dereference<T, decltype((void)*std::declval<T>())> : std::true_type { };
> 
> // test the test
> static_assert( has_dereference<int*>::value, "");
> static_assert(!has_dereference<int>::value, "");
> static_assert(!has_dereference<void*>::value, "");
> 
> // make sure std::unique_ptr<void>::operator* is SFINAE-friendly
> static_assert(has_dereference<std::unique_ptr<void>>::value, "");
> static_assert(noexcept(*std::declval<std::unique_ptr<void>>()), "");
> ```
> 
> And then the implementation should handle `void` in all standard modes, because the problem is detectable with SFINAE in all modes:
> 
> ```
> #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
> 
> LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX23 __add_lvalue_reference_t<_Tp> operator*() const
>   _NOEXCEPT(__is_noexcept_deref_or_void<pointer>::value)
> {...}
> ```
Since we can't use `has_dereference` in the `noexcept` I wonder why we need these tests. It was useful during the discussion, but only adding the second code block seems sufficient to me.


================
Comment at: libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/dereference.single.pass.cpp:21
+#if TEST_STD_VER >= 11
+struct ThrowDereference {
+  TEST_CONSTEXPR_CXX23 ThrowDereference& operator*() noexcept(false);
----------------
ldionne wrote:
> As you mentioned, you could also add a few more test cases with user-defined pointer types. For example a `noexcept(true)` user-defined type.
> 
> Also with builtin types, e.g. `noexcept(*declval<unique_ptr<int>>())`.
I added a few. I thought `operator*` had special proxy logic, but that's only for `operator->`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128214/new/

https://reviews.llvm.org/D128214