Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

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

Authored by Mordante on Jun 20 2022, 9:08 AM.

Details

Reviewers
philnik
ldionne
Group Reviewers
Restricted Project
Summary

This implements

  • LWG2762 unique_ptr operator*() should be noexcept.

Diff Detail

Event Timeline

Mordante created this revision.Jun 20 2022, 9:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2022, 9:08 AM
Mordante requested review of this revision.Jun 20 2022, 9:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2022, 9:08 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik accepted this revision as: philnik.Jun 20 2022, 9:21 AM
philnik added a subscriber: 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
270–286

Maybe replace _LIBCPP_INLINE_VISIBILITY while you're at it?

271

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

Mordante planned changes to this revision.Jun 20 2022, 10:20 AM

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
271

Good point.

Mordante updated this revision to Diff 438773.Jun 21 2022, 11:18 AM

An experiment to fix the CI issue.

Mordante planned changes to this revision.Jun 21 2022, 11:21 AM
Mordante marked an inline comment as done.
Mordante added inline comments.
libcxx/include/__memory/unique_ptr.h
275

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

Mordante updated this revision to Diff 438779.Jun 21 2022, 11:26 AM

tab -> space.

Mordante updated this revision to Diff 520193.May 7 2023, 10:04 AM

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

Mordante updated this revision to Diff 520199.May 7 2023, 10:39 AM

CI fixes.

Mordante updated this revision to Diff 520203.May 7 2023, 11:21 AM

CI fixes.

Mordante updated this revision to Diff 555095.Aug 31 2023, 10:29 AM

Rebased to trigger CI.

Mordante updated this revision to Diff 556364.Sep 10 2023, 3:32 AM

Rebased to trigger CI.

ldionne requested changes to this revision.Sep 12 2023, 10:26 AM
ldionne added a subscriber: ldionne.
ldionne added inline comments.
libcxx/include/__memory/unique_ptr.h
269–286

_LIBCPP_HIDE_FROM_ABI while we're at it.

281

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.

This revision now requires changes to proceed.Sep 12 2023, 10:26 AM
Mordante updated this revision to Diff 557948.Tue, Oct 31, 11:55 AM
Mordante marked 3 inline comments as done.

Rebased and address review comments.

libcxx/include/__memory/unique_ptr.h
269–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.

270–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.

281

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 accepted this revision.Fri, Nov 3, 10:54 AM

LGTM once the comments have been addressed.

libcxx/include/__memory/unique_ptr.h
269–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
450–451

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

This revision is now accepted and ready to land.Fri, Nov 3, 10:54 AM
Mordante marked 3 inline comments as done.Mon, Nov 6, 10:14 AM
Mordante added inline comments.
libcxx/include/__memory/unique_ptr.h
269–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 updated this revision to Diff 558031.Mon, Nov 6, 10:15 AM
Mordante marked an inline comment as done.

Rebased and addresses review comments.

Mordante updated this revision to Diff 558032.Mon, Nov 6, 11:17 AM

C++03 fixes.

ldionne added inline comments.Wed, Nov 8, 1:37 PM
libcxx/include/__memory/unique_ptr.h
34

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.

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

Can we use _NOEXCEPT_(...) instead?