[libcxx-commits] [PATCH] D63230: Add observer_ptr

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added a comment.

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.



================
Comment at: include/experimental/memory:49
+
+#include <type_traits>
+#include <functional>
----------------
The first include should always be either `<__config>`, or when in experimental, `<experimental/__config>`.


================
Comment at: include/experimental/memory:60
+#include <__undef_macros>
+
+
----------------
This header should ifdef everything out when `_LIBCPP_STD_VER <= 14`. I would put these guards just inside the namespace declaration.


================
Comment at: include/experimental/memory:62
+
+_LIBCPP_BEGIN_NAMESPACE_STD // TODO: use _LIBCPP_BEGIN_NAMESPACE_LFTS?
+
----------------
Yes. You *cannot* add additional symbols to namespace `std`


================
Comment at: include/experimental/memory:67
+{
+    typedef add_pointer_t<_Wp>          pointer;
+    typedef add_lvalue_reference_t<_Wp> reference;
----------------
These should be reserved names if they're not part of the spec.


================
Comment at: include/experimental/memory:81
+    template<class _W2,
+             class = enable_if_t<is_convertible<_W2*, _Wp*>::value>>
+    constexpr observer_ptr(observer_ptr<_W2> __other) noexcept
----------------
Use the new `_EnableIf` trait instead of `enable_if_t`.


================
Comment at: include/experimental/memory:95
+    // conversions
+    constexpr /*explicit*/ operator pointer() const noexcept { return __ptr; }
+
----------------
Why is `explicit` commented out`?




================
Comment at: include/experimental/memory:111
+template<class _Wp>
+void swap(observer_ptr<_Wp>& __a, observer_ptr<_Wp>& __b)
+{
----------------
I think we probably want to `noexcept` each of these free functions.



================
Comment at: test/libcxx/experimental/memory/memory.observer.ptr/version.pass.cpp:11
+
+#include <memory>
+
----------------
Wrong header?


================
Comment at: test/std/experimental/memory/memory.observer.ptr/hash.pass.cpp:33
+    {
+        test_hash_enabled_for_type<std::shared_ptr<int>>();
+        test_hash_enabled_for_type<std::shared_ptr<A>>();
----------------
Copy paste error?


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

https://reviews.llvm.org/D63230