[libcxx-commits] [PATCH] D50106: [libc++] Fix tuple assignment from types derived from a tuple-like
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Feb 22 07:41:17 PST 2021
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;
> 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<
> 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<
> 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.
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the libcxx-commits