Page MenuHomePhabricator

Add observer_ptr
Needs ReviewPublic

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

Details

Summary

This patch fixes 2516. It also adds observer_ptr (n4282).

Questions:

  • Is it okay to use constexpr and noexcept or should I use the macros?
  • Should this use the experimental namespace? The paper made it seem like std was sufficient.
  • I think there is something else I need to do when adding a header.
  • The greater than operator seemed wrong.

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
50

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

61

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

63

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

68

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

82

Use the new _EnableIf trait instead of enable_if_t.

96

Why is explicit commented out`?

112

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

test/libcxx/experimental/memory/memory.observer.ptr/version.pass.cpp
12

Wrong header?

test/std/experimental/memory/memory.observer.ptr/hash.pass.cpp
34

Copy paste error?

xbolva00 added inline comments.
include/experimental/memory
169

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
169

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
82

Will do, but any specific reason for this?

96

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
50

Good to know, thanks.

63

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

68

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