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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__memory/unique_ptr.h:270
 
-  _LIBCPP_INLINE_VISIBILITY
-  typename add_lvalue_reference<_Tp>::type
-  operator*() const {
+  _LIBCPP_INLINE_VISIBILITY typename add_lvalue_reference<_Tp>::type
+  operator*() const _NOEXCEPT_(_NOEXCEPT_(*declval<pointer>())) {
----------------
philnik wrote:
> Maybe replace `_LIBCPP_INLINE_VISIBILITY` while you're at it?
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.


================
Comment at: libcxx/include/__memory/unique_ptr.h:272
 
-  _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
----------------
ldionne wrote:
> `_LIBCPP_HIDE_FROM_ABI` while we're at it.
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.


================
Comment at: libcxx/include/__memory/unique_ptr.h:284
+#else  //  _LIBCPP_STD_VER >= 20
+      _NOEXCEPT_(noexcept(*std::declval<pointer>()))
+#endif //  _LIBCPP_STD_VER >= 20
----------------
ldionne wrote:
> I don't understand. Why is the `void*` case not important pre-C++20?
The issue it triggers is C++20 only. It's in the `__cpp17_iterator` concept so that will not be used before C++20.



================
Comment at: libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.observers/dereference.single.pass.cpp:27
+struct Deleter {
+  using pointer = ThrowDereference;
+  TEST_CONSTEXPR_CXX23 void operator()(ThrowDereference&) const {}
----------------
ldionne wrote:
> I don't think this typedef is needed.
It's needed to propagate the noexcept status, see line 40.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128214