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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__memory/unique_ptr.h:288
+#ifndef _LIBCPP_CXX03_LANG
+      noexcept(__is_noexcept_deref_or_void<pointer>::value)
+#endif
----------------
Can we use `_NOEXCEPT_(...)` instead?


================
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*.
----------------
Mordante wrote:
> 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.
Can we add these tests though?

```
// 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>>()), "");
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128214