This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Fix tuple assignment from types derived from a tuple-like
ClosedPublic

Authored by ldionne on Jul 31 2018, 2:38 PM.

Details

Summary

The implementation of tuple's constructors and assignment operators
currently diverges from the way the Standard specifies them, which leads
to subtle cases where the behavior is not as specified. In particular, a
class derived from a tuple-like type (e.g. pair) can't be assigned to a
tuple with corresponding members, when it should. This commit re-implements
the assignment operators (BUT NOT THE CONSTRUCTORS) in a way much closer
to the specification to get rid of this bug. Most of the tests have been
stolen from Eric's patch https://reviews.llvm.org/D27606.

As a fly-by improvement, tests for noexcept correctness have been added
to all overloads of operator=. We should tackle the same issue for the
tuple constructors in a future patch - I'm just trying to make progress
on fixing this long-standing bug.

PR17550
rdar://15837420

Diff Detail

Event Timeline

ldionne created this revision.Jul 31 2018, 2:38 PM
vsk added a subscriber: vsk.Aug 7 2018, 10:48 AM

Ping! It would be nice to land this in LLVM 8

EricWF requested changes to this revision.Jan 10 2019, 2:51 AM

Our constructors still have the same bug,. Are you planning on fixing those as well? Doing so will require a metric butt-tonne of overloads.
If you're not planning on fixing the constructors, then can you explain why it's better that we're inconsistent?

Otherwises, this patch looks mostly OK. However, it makes a bunch of previously lazy SFINAE eager. Here are some tests:
https://gist.github.com/EricWF/88ceadf2bcdeef9f9d268b3a743dcd04

Is there a reason we can't use __tuple_assignable anymore?

This revision now requires changes to proceed.Jan 10 2019, 2:51 AM

Our constructors still have the same bug,. Are you planning on fixing those as well? Doing so will require a metric butt-tonne of overloads.
If you're not planning on fixing the constructors, then can you explain why it's better that we're inconsistent?

We should fix the constructors too, but one thing at a time. We should have the 18 constructors that are mandated by the Standard, I think.

Yes, I hate that.

Otherwises, this patch looks mostly OK. However, it makes a bunch of previously lazy SFINAE eager. Here are some tests:
https://gist.github.com/EricWF/88ceadf2bcdeef9f9d268b3a743dcd04

Good catch! Genuine question: In the Standard, we say in http://eel.is/c++draft/tuple.assign#10 (for tuple<Types...>::operator=(tuple<UTypes...> const&)):

Remarks: This operator shall not participate in overload resolution unless sizeof...(Types) == sizeof...(UTypes) and is_­assignable_­v<Ti&, Ui const&> is true for all i.

This makes me wonder whether short-circuiting is mandated by the Standard. It certainly is a good QOI property to have it, but it's not clear to me this is required. WDYT?

Is there a reason we can't use __tuple_assignable anymore?

We're not using __tuple_types, but there's no fundamental reason for that. I found it easier to have the logic inline. I do think we need to get back the short-circuiting behaviour that was there before (using your insanely clever SFINAE trick for left-to-right evaluation in a parameter pack expansion).

ldionne updated this revision to Diff 322746.Feb 10 2021, 10:44 AM
ldionne retitled this revision from [libc++] Fix tuple assignment from type derived from a tuple-like to [libc++] Fix tuple assignment from types derived from a tuple-like.
ldionne edited the summary of this revision. (Show Details)

Fix issues with the laziness of how we evaluate type traits

Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2021, 10:44 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne updated this revision to Diff 323047.Feb 11 2021, 9:18 AM

Fix segfault on GCC and a few incorrect checks in the type traits.

I reduced the GCC segfault and it appears to have been fixed on GCC trunk, so
I won't be filing a bug report.

Quuxplusone added inline comments.
libcxx/include/tuple
432

Your friendly neighborhood ADL hater says: _VSTD::__swallow plz
(and line 438)
(and maybe int() is a sillier-than-necessary way to write 0?)

891

Your friendly neighborhood ADL hater says: _VSTD:: plz
(and line 900)
(and line 931)
(and line 986)
(and line 1002)

ldionne updated this revision to Diff 324645.Feb 18 2021, 8:09 AM

Fix build on GCC (or at least I think it should)

ldionne marked 2 inline comments as done.Feb 18 2021, 8:12 AM
ldionne updated this revision to Diff 324648.Feb 18 2021, 8:13 AM

Address Arthur's comments.

ldionne updated this revision to Diff 325039.Feb 19 2021, 10:56 AM

Fix GCC issues. I swear I had done the same fix locally previously, but I must
have messed something up with Git or arc diff.

Quuxplusone accepted this revision.Feb 19 2021, 2:40 PM

Suggested some ways to improve test coverage, and continued bikeshedding on the SFINAE ;) but there's no reason to hold this up AFAIC.

libcxx/include/tuple
58

Since this already isn't mimicking the syntax of http://eel.is/c++draft/tuple.tuple#tuple.assign-1 , I think you should say

tuple& operator=(tuple&&) noexcept(is_nothrow_move_assignable_v<T> && ...);

but it definitely doesn't matter.

975

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.

992

Nitpick: Sent you a Slack suggesting how to eliminate this class = void.

libcxx/test/libcxx/utilities/tuple/tuple.tuple/tuple.assign/const_array.pass.cpp
53 ↗(On Diff #325039)

I would like to see this file combined with rvalue_array.pass.cpp, and also test the currently missing two value categories:

static_assert(std::is_nothrow_assignable<Tuple&, Array&>::value, "");
static_assert(std::is_nothrow_assignable<Tuple&, const Array&&>::value, "");

and also the negative cases:

static_assert(!std::is_assignable<Tuple&, std::array<long,2>&>::value, "");
static_assert(!std::is_assignable<Tuple&, std::array<long,2>&&>::value, "");
static_assert(!std::is_assignable<Tuple&, const std::array<long,2>&>::value, "");
static_assert(!std::is_assignable<Tuple&, const std::array<long,2>&&>::value, "");

static_assert(!std::is_assignable<Tuple&, std::array<long,4>&>::value, "");
static_assert(!std::is_assignable<Tuple&, std::array<long,4>&&>::value, "");
static_assert(!std::is_assignable<Tuple&, const std::array<long,4>&>::value, "");
static_assert(!std::is_assignable<Tuple&, const std::array<long,4>&&>::value, "");
libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/const_pair.pass.cpp
52

Don't just test for lack-of-nothrow-assignability; test for assignability-but-maythrow:

static_assert(std::is_assignable<Tuple&, Pair const&>::value, "");
static_assert(!std::is_nothrow_assignable<Tuple&, Pair const&>::value, "");

And as above, I'd prefer to see tests for Pair& and Pair&& and const Pair&& as well.

libcxx/include/tuple
971

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!)

ldionne updated this revision to Diff 325445.Feb 22 2021, 7:39 AM
ldionne marked 6 inline comments as done.

Address Arthur's comments.

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

libcxx/include/tuple
971

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.

975

The goal is to remove the extension shortly after.

992

Replied there. The short story is that I used class = void to workaround the GCC segfaults I was seeing on the bots.

ldionne updated this revision to Diff 325449.Feb 22 2021, 7:58 AM

Rebase onto main.

ldionne accepted this revision as: Restricted Project.Feb 22 2021, 11:51 AM
This revision was not accepted when it landed; it landed in state Needs Review.Feb 22 2021, 11:52 AM
This revision was automatically updated to reflect the committed changes.