This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Implement std::experimental::observer_ptr
ClosedPublic

Authored by ldionne on Jun 12 2019, 3:28 PM.

Details

Summary

This patch adds std::experimental::observer_ptr (n4282) and also
fixes LWG2516.

Diff Detail

Event Timeline

zoecarver created this revision.Jun 12 2019, 3:28 PM
zoecarver updated this revision to Diff 204683.Jun 13 2019, 7:01 PM
  • fix overload resolution

Questions:

Is it okay to use constexpr and noexcept or should I use the macros?

Yes. For entirely new features, all of C++11 and C++14 are on the table now.
But for additions or changes to existing components, use the same style as the existing code.

Should this use the experimental namespace? The paper made it seem like std was sufficient.

We implement a C++ standard, what namespace does it say?

I think there is something else I need to do when adding a header.

There's a set up steps here:
https://github.com/llvm-mirror/libcxx/blob/master/NOTES.TXT#L19

But generally it looked good.

The greater than operator seemed wrong.

Say more?

  • new header things

Questions:

Is it okay to use constexpr and noexcept or should I use the macros?

Yes. For entirely new features, all of C++11 and C++14 are on the table now.
But for additions or changes to existing components, use the same style as the existing code.

Good to know. Thanks.

I think there is something else I need to do when adding a header.

There's a set up steps here:
https://github.com/llvm-mirror/libcxx/blob/master/NOTES.TXT#L19

But generally it looked good.

Done.

The greater than operator seemed wrong.

Say more?

The standard says that the greater than and less than operators should have the same functionality. This seems wrong to me, but I might be missing something.

The implementation looks great!

I've added some inline comments, but I think the biggest remaining issue is the lack of constexpr tests. If a function is declared constexpr, we should have a test for it.

Let me know if you have any questions.

include/experimental/memory
49 ↗(On Diff #204709)

The first include should always be either <__config>, or when in experimental, <experimental/__config>.

60 ↗(On Diff #204709)

This header should ifdef everything out when _LIBCPP_STD_VER <= 14. I would put these guards just inside the namespace declaration.

62 ↗(On Diff #204709)

Yes. You *cannot* add additional symbols to namespace std

67 ↗(On Diff #204709)

These should be reserved names if they're not part of the spec.

81 ↗(On Diff #204709)

Use the new _EnableIf trait instead of enable_if_t.

95 ↗(On Diff #204709)

Why is explicit commented out`?

111 ↗(On Diff #204709)

I think we probably want to noexcept each of these free functions.

test/libcxx/experimental/memory/memory.observer.ptr/version.pass.cpp
11 ↗(On Diff #204709)

Wrong header?

test/std/experimental/memory/memory.observer.ptr/hash.pass.cpp
33 ↗(On Diff #204709)

Copy paste error?

xbolva00 added inline comments.
include/experimental/memory
168 ↗(On Diff #204709)

Standard says "Returns: p2 < p1.".

So in your case, "b < a" ?

zoecarver marked an inline comment as done.Jun 23 2019, 8:21 PM

I am so glad you like it :) thanks for the review @EricWF! I will add constexpr tests and fix the comments.

include/experimental/memory
168 ↗(On Diff #204709)

How did I miss this... I read over it so many times. Anyway, thank you @xbolva00.

zoecarver marked 7 inline comments as done.Jun 30 2019, 12:09 PM
zoecarver added inline comments.
include/experimental/memory
81 ↗(On Diff #204709)

Will do, but any specific reason for this?

95 ↗(On Diff #204709)

I would expect is_convertible< observer_ptr<T>, T*> to be true, but when this is explicit that statement is not true.

zoecarver marked 7 inline comments as done.Jun 30 2019, 12:23 PM
zoecarver added inline comments.
include/experimental/memory
49 ↗(On Diff #204709)

Good to know, thanks.

62 ↗(On Diff #204709)

You're right. That is what the standard says. I was confused by the paper.

67 ↗(On Diff #204709)

Based on 2516 it was unclear to me whether these should still exist or should be mangled.

zoecarver marked 2 inline comments as done.
  • updated based on your comments
  • added a few constexpr tests

My main question with adding constexpr is in swap. I am not sure how to make a swap function that is constexpr.

zoecarver updated this revision to Diff 207469.Jul 1 2019, 8:20 PM
  • add more constexpr tests

Friendly ping.

ldionne added a reviewer: Restricted Project.Nov 2 2020, 2:23 PM
qiucf added a subscriber: qiucf.Mar 24 2021, 12:09 AM
ldionne commandeered this revision.Sep 11 2023, 3:24 PM
ldionne edited reviewers, added: zoecarver; removed: ldionne.

[Github PR transition cleanup]

Commandeering to pick up.

Herald added a project: Restricted Project. · View Herald TranscriptSep 11 2023, 3:24 PM
ldionne updated this revision to Diff 556492.Sep 11 2023, 3:25 PM
ldionne retitled this revision from Add observer_ptr to [libc++] Implement std::experimental::observer_ptr.
ldionne edited the summary of this revision. (Show Details)

Rebase, fix a few things, clang-format.

Herald added a project: Restricted Project. · View Herald TranscriptSep 11 2023, 3:25 PM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
ldionne updated this revision to Diff 556511.Sep 11 2023, 5:35 PM

Rebase on top of latest spec, revisit tests.

ldionne updated this revision to Diff 556962.Sep 18 2023, 11:08 AM

Add missing _LIBCPP_HIDE_FROM_ABI and fix transitive includes.

ldionne updated this revision to Diff 557338.Sep 25 2023, 3:36 PM

Add missing include to test.

ldionne updated this revision to Diff 557361.Sep 26 2023, 6:58 AM

Fix formatting.

ldionne updated this revision to Diff 557897.Oct 26 2023, 8:23 AM

Fix C++17 test.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 5 2023, 5:01 PM
This revision was automatically updated to reflect the committed changes.