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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
