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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const marked 3 inline comments as done.
var-const added a comment.

Thanks for the comments, everyone! PTAL.



================
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
----------------
Quuxplusone wrote:
> 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.)
Thanks! This is also exactly what @ldionne suggested.

I'm not sure if further backporting can be done without a proposal?


================
Comment at: libcxx/test/std/utilities/memory/util.smartptr/util.smartptr.weak/types.pass.cpp:33
+#if TEST_STD_VER > 17
+  static_assert(
+      std::is_same<typename std::weak_ptr<T[]>::element_type, T>::value, "");
----------------
jloser wrote:
> Why not use `ASSERT_SAME_TYPE` to be consistent instead of mixed use of `ASSERT_SAME_TYPE` and `std::is_same`? BTW, you could use `std::is_same_v` since this block is C++17 code.
Thanks for pointing it out, it's mostly because I copied it from a different file and didn't think to update it. Changed to use the macro both here and in the `shared_ptr` test this is copied from.


================
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>();
----------------
Quuxplusone wrote:
> 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.
Thanks for the suggestion. I added this check to the body of `main` (adding the check to `test` doesn't immediately work because the `ASSERT_SAME_TYPE(typename std::weak_ptr<T>::element_type, T)` check presumes the type is not an array, and it seemed easier to test this separately rather than find a workaround).

The `unique_ptr` test has a different structure, and I'd also prefer to keep this patch focused on shared/weak pointers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112092