Page MenuHomePhabricator

[libcxx] Fix tuple construction/assignment from types derived from tuple/pair/array.
ClosedPublic

Authored by EricWF on Dec 8 2016, 7:39 PM.

Details

Summary

The standard requires tuple have the following constructors:

tuple(tuple<OtherTypes...> const&);
tuple(tuple<OtherTypes...> &&);
tuple(pair<T1, T2> const&);
tuple(pair<T1, T2> &&);
tuple(array<T, N> const&);
tuple(array<T, N> &&);

However libc++ implements these as a single constructor with the signature:

template <class TupleLike, enable_if_t<__is_tuple_like<TupleLike>::value>>
tuple(TupleLike&&);

This causes the constructor to reject types derived from tuple-like types; Unlike if we had all of the concrete overloads, because they cause the derived->base conversion in the signature.

This patch fixes this issue by detecting derived types and the tuple-like base they are derived from. It does this by creating an overloaded function with signatures for each of tuple/pair/array and checking if the possibly derived type can convert to any of them.

This patch fixes PR17550

This patch

Diff Detail

Repository
rL LLVM

Event Timeline

EricWF updated this revision to Diff 80855.Dec 8 2016, 7:39 PM
EricWF retitled this revision from to [libcxx] Fix tuple construction/assignment from types derived from tuple/pair/array..
EricWF updated this object.
EricWF added reviewers: mclow.lists, K-ballo, mpark.
EricWF added a subscriber: cfe-commits.
EricWF updated this revision to Diff 80862.Dec 8 2016, 7:57 PM

Cleanup a couple of issues.

EricWF updated this revision to Diff 80870.Dec 9 2016, 2:10 AM

Fix comments.

mpark added inline comments.Dec 13 2016, 1:10 PM
include/tuple
568 ↗(On Diff #80870)

indentation?

871 ↗(On Diff #80870)

indentation?

906 ↗(On Diff #80870)

s/_Deducer/_Deduced/

907 ↗(On Diff #80870)

s/_QualTupleBase/_TupBase/

915 ↗(On Diff #80870)

A general comment about using the base in noexcept condition. I remember for variant we wanted to express it directly rather than delegating it to the "base". Does that also apply here?

917 ↗(On Diff #80870)

Here and elsewhere, why do we bother with base_.operator=(...); as opposed to just base_ = ...;?

mpark requested changes to this revision.Dec 13 2016, 1:26 PM
mpark edited edge metadata.
This revision now requires changes to proceed.Dec 13 2016, 1:26 PM
EricWF updated this revision to Diff 81456.Dec 14 2016, 1:32 PM
EricWF edited edge metadata.
EricWF marked 4 inline comments as done.

Address review comments.

@mpark any other concerns/comments?

EricWF accepted this revision.Dec 14 2016, 2:31 PM
EricWF added a reviewer: EricWF.

Accepting revision. @mpark mentioned that this was good to go offline. Ignore the inline comment about base.operator=, phab won't let me delete it.

include/tuple
884 ↗(On Diff #80870)

This should be fixed

915 ↗(On Diff #80870)

Yes. probably. but this is existing code. I'll fix that in another patch. Plus it's really hard to state this noexcept directly since you need to expand the input argument into its tuple-types.

917 ↗(On Diff #80870)

I think the reason for not using base_ = ... is because that can select different conversion sequences IIRC.

This revision was automatically updated to reflect the committed changes.