[libc++] Makes `unique_ptr operator*() noexcept.

[Mordante]

Summary

This implements

• LWG2762 unique_ptr operator*() should be noexcept.

[philnik]

Could you include in the title that this isn't just modifying unique_ptr? Other than a few nits this LGTM assuming CI passes.

libcxx/include/__memory/unique_ptr.h

272–286

Maybe replace _LIBCPP_INLINE_VISIBILITY while you're at it?

273

It gets removed if noexcept() isn't supported anyways.

[Mordante]

Thanks for the review!

However I will put this on hold. The CI failure seems to indicate there's an issue with the LWG issue. GCC agrees with Clang so I need to do some more investigation.

libcxx/include/__memory/unique_ptr.h

273

Good point.

[Mordante]

An experiment to fix the CI issue.

[Mordante]

libcxx/include/__memory/unique_ptr.h

277

Note this is just an experiment and not intended to be landed in this fashion.

[Mordante]

tab -> space.

[Mordante]

Poke CI.

[Mordante]

Poke CI.

[Mordante]

Rebased, resolved merge conflicts, and updated to current libc++ style.

[Mordante]

CI fixes.

[Mordante]

CI fixes.

[Mordante]

Rebased to trigger CI.

[Mordante]

Rebased to trigger CI.

[ldionne]

libcxx/include/__memory/unique_ptr.h

272–286

_LIBCPP_HIDE_FROM_ABI while we're at it.

284

I don't understand. Why is the void* case not important pre-C++20?

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/dereference.single.pass.cpp

27

I don't think this typedef is needed.

[Mordante]

Rebased and address review comments.

libcxx/include/__memory/unique_ptr.h

272–286

I really prefer to do that in a larger patch. Maybe something to discuss with the team what we want to do with this and how to move forward removing this techdebt.

I wouldn't mind to do it after this patch for the entire file, including _VSTD and friends too.

272–286

I really prefer to do that in a larger patch. Maybe something to discuss with the team what we want to do with this and how to move forward removing this techdebt.

I wouldn't mind to do it after this patch for the entire file, including _VSTD and friends too.

284

The issue it triggers is C++20 only. It's in the __cpp17_iterator concept so that will not be used before C++20.

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/dereference.single.pass.cpp

27

It's needed to propagate the noexcept status, see line 40.

[ldionne]

LGTM once the comments have been addressed.

libcxx/include/__memory/unique_ptr.h

272–287

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)
{...}

libcxx/include/memory

452–455

We can probably drop the typename from the synopsis?

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/dereference.single.pass.cpp

21

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>>()).

[Mordante]

libcxx/include/__memory/unique_ptr.h

272–287

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.

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/dereference.single.pass.cpp

21

I added a few. I thought operator* had special proxy logic, but that's only for operator->.

[Mordante]

Rebased and addresses review comments.

[Mordante]

C++03 fixes.

[ldionne]

libcxx/include/__memory/unique_ptr.h

52

This comment seems a bit unnecessary to me once we add tests for void. Then it's pretty self explanatory, if you try to remove this the test would fail.

272–287

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

274

Can we use _NOEXCEPT_(...) instead?

libcxx/docs/Status/Cxx2bIssues.csv

 `3549 <https://wg21.link/LWG3549>`__,"``view_interface`` is overspecified to derive from ``view_base``","June 2021","|Complete|","14.0","|ranges|"
 `3551 <https://wg21.link/LWG3551>`__,"``borrowed_{iterator,subrange}_t`` are overspecified","June 2021","|Nothing To Do|","","|ranges|"
 `3552 <https://wg21.link/LWG3552>`__,"Parallel specialized memory algorithms should require forward iterators","June 2021","",""
 `3553 <https://wg21.link/LWG3553>`__,"Useless constraint in ``split_view::outer-iterator::value_type::begin()``","June 2021","","","|ranges|"
 `3555 <https://wg21.link/LWG3555>`__,"``{transform,elements}_view::iterator::iterator_concept`` should consider const-qualification of the underlying range","June 2021","","","|ranges|"
 "","","","","",""
 `2191 <https://wg21.link/LWG2191>`__,"Incorrect specification of ``match_results(match_results&&)``","October 2021","|Nothing To Do|",""
 `2381 <https://wg21.link/LWG2381>`__,"Inconsistency in parsing floating point numbers","October 2021","",""
-`2762 <https://wg21.link/LWG2762>`__,"``unique_ptr operator*()`` should be ``noexcept``","October 2021","",""
+`2762 <https://wg21.link/LWG2762>`__,"``unique_ptr operator*()`` should be ``noexcept``","October 2021","|Complete|","17.0"
 `3121 <https://wg21.link/LWG3121>`__,"``tuple`` constructor constraints for ``UTypes&&...`` overloads","October 2021","",""
 `3123 <https://wg21.link/LWG3123>`__,"``duration`` constructor from representation shouldn't be effectively non-throwing","October 2021","","","|chrono|"
 `3146 <https://wg21.link/LWG3146>`__,"Excessive unwrapping in ``std::ref/cref``","October 2021","|Complete|","14.0"
 `3152 <https://wg21.link/LWG3152>`__,"``common_type`` and ``common_reference`` have flaws in common","October 2021","",""
 `3293 <https://wg21.link/LWG3293>`__,"``move_iterator operator+()`` has incorrect constraints","October 2021","|Complete|","15.0"
 `3361 <https://wg21.link/LWG3361>`__,"``safe_range<SomeRange&>`` case","October 2021","|Nothing To Do|","","|ranges|"
 `3392 <https://wg21.link/LWG3392>`__,"``ranges::distance()`` cannot be used on a move-only iterator with a sized sentinel","October 2021","|Complete|","14.0","|ranges|"
 `3407 <https://wg21.link/LWG3407>`__,"Some problems with the wording changes of P1739R4","October 2021","|Complete|","15.0","|ranges|"

libcxx/include/__memory/unique_ptr.h

 #include <__type_traits/is_default_constructible.h>
 #include <__type_traits/is_function.h>
 #include <__type_traits/is_pointer.h>
 #include <__type_traits/is_reference.h>
 #include <__type_traits/is_same.h>
 #include <__type_traits/is_swappable.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>
 #include <__utility/move.h>
 #include <cstddef>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 template <class _Tp>
 struct _LIBCPP_TEMPLATE_VIS default_delete {
     static_assert(!is_function<_Tp>::value,
                   "default_delete cannot be instantiated for function types");
 #ifndef _LIBCPP_CXX03_LANG
   _LIBCPP_INLINE_VISIBILITY constexpr default_delete() _NOEXCEPT = default;
 #else
   _LIBCPP_INLINE_VISIBILITY default_delete() {}
 #endif
   template <class _Up>
#endif
 
   _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX23 ~unique_ptr() { reset(); }
 
   _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX23 unique_ptr& operator=(nullptr_t) _NOEXCEPT {
     reset();
     return *this;
   }
 
-  _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX23 __add_lvalue_reference_t<_Tp> operator*() const {
+  _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*.
+          // The issue was triggered by
+          // test/std/utilities/memory/unique.ptr/iterator_concept_conformance.compile.pass.cpp
+          return noexcept(*std::declval<pointer>());
+      }())
+#else  //  _LIBCPP_STD_VER >= 20
+      _NOEXCEPT_(noexcept(*std::declval<pointer>()))
+#endif //  _LIBCPP_STD_VER >= 20
+  {
     return *__ptr_.first();
   }
   _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX23 pointer operator->() const _NOEXCEPT {
     return __ptr_.first();
   }
   _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX23 pointer get() const _NOEXCEPT { return __ptr_.first(); }
   _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX23 deleter_type& get_deleter() _NOEXCEPT {
     return __ptr_.second();
   }

libcxx/include/memory

public:
     constexpr ~unique_ptr();                                     // constexpr since C++23
 
     // assignment
     constexpr unique_ptr& operator=(unique_ptr&& u) noexcept;                         // constexpr since C++23
     template <class U, class E>
     constexpr unique_ptr& operator=(unique_ptr<U, E>&& u) noexcept;                   // constexpr since C++23
     constexpr unique_ptr& operator=(nullptr_t) noexcept;                              // constexpr since C++23
 
     // observers
-    typename constexpr add_lvalue_reference<T>::type operator*() const;               // constexpr since C++23
+    typename constexpr
+    add_lvalue_reference<T>::type operator*() const noexcept(see below);              // constexpr since C++23
     constexpr pointer operator->() const noexcept;                                    // constexpr since C++23
     constexpr pointer get() const noexcept;                                           // constexpr since C++23
     constexpr deleter_type& get_deleter() noexcept;                                   // constexpr since C++23
     constexpr const deleter_type& get_deleter() const noexcept;                       // constexpr since C++23
     constexpr explicit operator bool() const noexcept;                                // constexpr since C++23
 
     // modifiers
     constexpr pointer release() noexcept;                                             // constexpr since C++23
     constexpr void reset(pointer p = pointer()) noexcept;                             // constexpr since C++23

libcxx/include/optional

public:
     template<class U> optional &operator=(optional<U> &&);                        // constexpr in C++20
     template<class... Args> T& emplace(Args &&...);                               // constexpr in C++20
     template<class U, class... Args> T& emplace(initializer_list<U>, Args &&...); // constexpr in C++20
 
     // [optional.swap], swap
     void swap(optional &) noexcept(see below ); // constexpr in C++20
 
     // [optional.observe], observers
-    constexpr T const *operator->() const;
-    constexpr T *operator->();
-    constexpr T const &operator*() const &;
-    constexpr T &operator*() &;
-    constexpr T &&operator*() &&;
-    constexpr const T &&operator*() const &&;
+    constexpr T const *operator->() const noexcept;
+    constexpr T *operator->() noexcept;
+    constexpr T const &operator*() const & noexcept;
+    constexpr T &operator*() & noexcept;
+    constexpr T &&operator*() && noexcept;
+    constexpr const T &&operator*() const && noexcept;
     constexpr explicit operator bool() const noexcept;
     constexpr bool has_value() const noexcept;
     constexpr T const &value() const &;
     constexpr T &value() &;
     constexpr T &&value() &&;
     constexpr const T &&value() const &&;
     template<class U> constexpr T value_or(U &&) const &;
     template<class U> constexpr T value_or(U &&) &&;
_LIBCPP_CONSTEXPR_SINCE_CXX20 void swap(optional& __opt)
                 __opt.reset();
             }
         }
     }
 
     _LIBCPP_INLINE_VISIBILITY
     constexpr
     add_pointer_t<value_type const>
-    operator->() const
+    operator->() const noexcept
     {
         _LIBCPP_ASSERT(this->has_value(), "optional operator-> called on a disengaged value");
         return _VSTD::addressof(this->__get());
     }
 
     _LIBCPP_INLINE_VISIBILITY
     constexpr
     add_pointer_t<value_type>
-    operator->()
+    operator->() noexcept
     {
         _LIBCPP_ASSERT(this->has_value(), "optional operator-> called on a disengaged value");
         return _VSTD::addressof(this->__get());
     }
 
     _LIBCPP_INLINE_VISIBILITY
     constexpr
     const value_type&

libcxx/test/std/utilities/optional/optional.object/optional.object.observe/dereference.pass.cpp

test()
     return (*opt).test();
 }
 
 int main(int, char**)
 {
     {
         optional<X> opt; ((void)opt);
         ASSERT_SAME_TYPE(decltype(*opt), X&);
-        LIBCPP_STATIC_ASSERT(noexcept(*opt));
-        // ASSERT_NOT_NOEXCEPT(*opt);
-        // FIXME: This assertion fails with GCC because it can see that
-        // (A) operator*() is constexpr, and
-        // (B) there is no path through the function that throws.
-        // It's arguable if this is the correct behavior for the noexcept
-        // operator.
-        // Regardless this function should still be noexcept(false) because
-        // it has a narrow contract.
+        ASSERT_NOEXCEPT(*opt);
     }
     {
         optional<X> opt(X{});
         assert((*opt).test() == 4);
     }
     static_assert(test() == 7, "");
 
     return 0;
 }

libcxx/test/std/utilities/optional/optional.object/optional.object.observe/dereference_const.pass.cpp

struct Y
     int test() const {return 2;}
 };
 
 int main(int, char**)
 {
     {
         const optional<X> opt; ((void)opt);
         ASSERT_SAME_TYPE(decltype(*opt), X const&);
-        LIBCPP_STATIC_ASSERT(noexcept(*opt));
-        // ASSERT_NOT_NOEXCEPT(*opt);
-        // FIXME: This assertion fails with GCC because it can see that
-        // (A) operator*() is constexpr, and
-        // (B) there is no path through the function that throws.
-        // It's arguable if this is the correct behavior for the noexcept
-        // operator.
-        // Regardless this function should still be noexcept(false) because
-        // it has a narrow contract.
+        ASSERT_NOEXCEPT(*opt);
     }
     {
         constexpr optional<X> opt(X{});
         static_assert((*opt).test() == 3, "");
     }
     {
         constexpr optional<Y> opt(Y{});
         assert((*opt).test() == 2);
     }
 
     return 0;
 }

libcxx/test/std/utilities/optional/optional.object/optional.object.observe/dereference_const_rvalue.pass.cpp

struct Y
     int test() const && {return 2;}
 };
 
 int main(int, char**)
 {
     {
         const optional<X> opt; ((void)opt);
         ASSERT_SAME_TYPE(decltype(*std::move(opt)), X const &&);
-        LIBCPP_STATIC_ASSERT(noexcept(*opt));
-        // ASSERT_NOT_NOEXCEPT(*std::move(opt));
-        // FIXME: This assertion fails with GCC because it can see that
-        // (A) operator*() is constexpr, and
-        // (B) there is no path through the function that throws.
-        // It's arguable if this is the correct behavior for the noexcept
-        // operator.
-        // Regardless this function should still be noexcept(false) because
-        // it has a narrow contract.
+        ASSERT_NOEXCEPT(*std::move(opt));
     }
     {
         constexpr optional<X> opt(X{});
         static_assert((*std::move(opt)).test() == 5, "");
     }
     {
         constexpr optional<Y> opt(Y{});
         assert((*std::move(opt)).test() == 2);
     }
 
     return 0;
 }

libcxx/test/std/utilities/optional/optional.object/optional.object.observe/dereference_rvalue.pass.cpp

test()
     return (*std::move(opt)).test();
 }
 
 int main(int, char**)
 {
     {
         optional<X> opt; ((void)opt);
         ASSERT_SAME_TYPE(decltype(*std::move(opt)), X&&);
-        LIBCPP_STATIC_ASSERT(noexcept(*opt));
-        // ASSERT_NOT_NOEXCEPT(*std::move(opt));
-        // FIXME: This assertion fails with GCC because it can see that
-        // (A) operator*() is constexpr, and
-        // (B) there is no path through the function that throws.
-        // It's arguable if this is the correct behavior for the noexcept
-        // operator.
-        // Regardless this function should still be noexcept(false) because
-        // it has a narrow contract.
+        ASSERT_NOEXCEPT(*std::move(opt));
     }
     {
         optional<X> opt(X{});
         assert((*std::move(opt)).test() == 6);
     }
     static_assert(test() == 7, "");
 
     return 0;
 }

libcxx/test/std/utilities/optional/optional.object/optional.object.observe/op_arrow.pass.cpp

test()
     return opt->test();
 }
 
 int main(int, char**)
 {
     {
         std::optional<X> opt; ((void)opt);
         ASSERT_SAME_TYPE(decltype(opt.operator->()), X*);
-        // ASSERT_NOT_NOEXCEPT(opt.operator->());
-        // FIXME: This assertion fails with GCC because it can see that
-        // (A) operator->() is constexpr, and
-        // (B) there is no path through the function that throws.
-        // It's arguable if this is the correct behavior for the noexcept
-        // operator.
-        // Regardless this function should still be noexcept(false) because
-        // it has a narrow contract.
+        ASSERT_NOEXCEPT(opt.operator->());
     }
     {
         optional<X> opt(X{});
         assert(opt->test() == 3);
     }
     {
         static_assert(test() == 3, "");
     }
 
     return 0;
 }

libcxx/test/std/utilities/optional/optional.object/optional.object.observe/op_arrow_const.pass.cpp

struct Z
     constexpr int test() const {return 1;}
 };
 
 int main(int, char**)
 {
     {
         const std::optional<X> opt; ((void)opt);
         ASSERT_SAME_TYPE(decltype(opt.operator->()), X const*);
-        // ASSERT_NOT_NOEXCEPT(opt.operator->());
-        // FIXME: This assertion fails with GCC because it can see that
-        // (A) operator->() is constexpr, and
-        // (B) there is no path through the function that throws.
-        // It's arguable if this is the correct behavior for the noexcept
-        // operator.
-        // Regardless this function should still be noexcept(false) because
-        // it has a narrow contract.
+        ASSERT_NOEXCEPT(opt.operator->());
     }
     {
         constexpr optional<X> opt(X{});
         static_assert(opt->test() == 3, "");
     }
     {
         constexpr optional<Y> opt(Y{});
         assert(opt->test() == 2);
     }
     {
         constexpr optional<Z> opt(Z{});
         static_assert(opt->test() == 1, "");
     }
 
     return 0;
 }

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/dereference.single.pass.cpp

 
 // test op*()
 
 #include <memory>
 #include <cassert>
 
 #include "test_macros.h"
 
+#if TEST_STD_VER >= 11
+struct ThrowDereference {
+  TEST_CONSTEXPR_CXX23 ThrowDereference& operator*() noexcept(false);
+  TEST_CONSTEXPR_CXX23 operator bool() const { return false; }
+};
+
+struct Deleter {
+  using pointer = ThrowDereference;
+  TEST_CONSTEXPR_CXX23 void operator()(ThrowDereference&) const {}
+};
+#endif
+
 TEST_CONSTEXPR_CXX23 bool test() {
+  {
     std::unique_ptr<int> p(new int(3));
     assert(*p == 3);
+    ASSERT_NOEXCEPT(*p);
+  }
+#if TEST_STD_VER >= 11
+  {
+    // The noexcept status of *unique_ptr<>::pointer should be propagated.
+    std::unique_ptr<ThrowDereference, Deleter> p;
+    ASSERT_NOT_NOEXCEPT(*p);
+  }
+#endif
 
   return true;
 }
 
 int main(int, char**) {
   test();
 #if TEST_STD_VER >= 23
   static_assert(test());
 #endif
 
   return 0;
 }