[libcxx-commits] [PATCH] D50106: [libc++] Fix tuple assignment from types derived from a tuple-like

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

I think we should be pretty much good to go now. Will let CI run just in case.



================
Comment at: libcxx/include/tuple:971
+        _VSTD::get<0>(*this) = _VSTD::forward<_Up1>(__pair.first);
+        _VSTD::get<1>(*this) = _VSTD::forward<_Up2>(__pair.second);
+        return *this;
----------------
Quuxplusone wrote:
> Oh, late-breaking nit: I would prefer to see `... = static_cast<_Up1&&>(__pair.first);` here, because `_Up1` is not being deduced according to forwarding-reference rules, and thus shouldn't be "forwarded."  Pragmatically I think `forward<_Up1>` ends up doing the right thing in all cases... but I don't think it's appropriate here. (Plus, we save one function template instantiation by omitting it!)
It actually occurs to me that we should really be using `_VSTD::move(__pair.first)` since we're taking a rvalue reference to the pair as an argument. I'll change it to that, please LMK if you disagree.


================
Comment at: libcxx/include/tuple:975
+
+    // EXTENSION
+    template<class _Up, size_t _Np, class = _EnableIf<
----------------
Quuxplusone wrote:
> Perhaps in a followup patch: could we delineate exactly what code is part of this extension and put it all under an `#if _LIBCPP_TUPLE_ARRAY_ASSIGNMENT` or something, such that we could say "this is the exact extent of the extension code" and maybe even get customers to try compiling with `-D_LIBCPP_TUPLE_ARRAY_ASSIGNMENT=0` to see what breaks?
> 
> I'm assuming that our goal here is to deprecate and remove this extension over time. But even if we support it forever, I think `#if` would be nicer than the `//EXTENSION` comments.
The goal is to remove the extension shortly after.


================
Comment at: libcxx/include/tuple:992
+    // EXTENSION
+    template<class _Up, size_t _Np, class = void, class = _EnableIf<
+        _And<
----------------
Quuxplusone wrote:
> Nitpick: Sent you a Slack suggesting how to eliminate this `class = void`.
Replied there. The short story is that I used `class = void` to workaround the GCC segfaults I was seeing on the bots.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50106