This patch adds std::experimental::observer_ptr (n4282) and also
fixes LWG2516.
Details
- Reviewers
mclow.lists EricWF zoecarver - Group Reviewers
Restricted Project - Commits
- rG7a62bee611f1: [libc++] Implement std::experimental::observer_ptr
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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#L19But 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? |
include/experimental/memory | ||
---|---|---|
168 ↗ | (On Diff #204709) | Standard says "Returns: p2 < p1.". So in your case, "b < a" ? |
- 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.