[libcxx-commits] [PATCH] D112092: [libc++] LWG3001: add `remove_extent_t` to `weak_ptr::element_type`.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__memory/shared_ptr.h:384-410
+template<class _Tp, class _Up>
+struct __weak_ptr_compatible_with
+#if _LIBCPP_STD_VER > 17
+    : is_convertible<remove_extent_t<_Tp>*, remove_extent_t<_Up>*> {};
+#else
+    : is_convertible<_Tp*, _Up*> {};
+#endif // _LIBCPP_STD_VER > 17
----------------
ldionne wrote:
> var-const wrote:
> > This is pretty verbose, but it seemed important to ensure no changes in behavior for each language version. Please let me know if you have any suggestions on how to improve this.
> Per offline review: Since this is a LWG issue we're fixing, we normally apply it in all standard modes. So you can fix `weak_ptr` in C++17 and above in-place.
It would be much simpler to just treat LWG3001 as a DR against C++17. The issue description of LWG3001 makes it very clear that the //intent// was always to `remove_extent_t` in C++17; that diff just got lost in the editorial shuffle. I'm not saying "do it," but I'm at least saying "I hope @ldionne tells you to do it." ;)

(Personally I would be interested in a followup PR that DR'ed the `remove_extent_t` change back to C++11 just for simplicity's sake, but I suppose there'd be no real justification for that.)


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.weak/types.pass.cpp:42
 {
-    static_assert((std::is_same<std::weak_ptr<A>::element_type, A>::value), "");
+  test<A>();
+  test<B>();
----------------
var-const wrote:
> I copied this from a similar test for `shared_ptr`.
In both places, I think it would be valuable to include `test<int[2]>`. We expect `foo_ptr<int[][2]>::element_type` to be `int[2]`. I could imagine someone accidentally flubbing it to `int` with a pretty simple typo (changing `remove_extent` into `remove_all_extents`).
If we have a similar test for `unique_ptr<X>::element_type`, this comment would apply there too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112092