Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Adding ITuple interface and implementation in ValueTuple #10068

Closed
wants to merge 4 commits into from
Closed

Adding ITuple interface and implementation in ValueTuple #10068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
34114f7
Adding ITuple implementation
jcouv Jul 14, 2016
176a739
Renaming from Size to Length
jcouv Jul 14, 2016
7589bb6
Using ITuple as constraint on TRest
jcouv Jul 14, 2016
c02d837
Removing boxing cast to ITuple
jcouv Jul 15, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/System.ValueTuple/src/System/ValueTuple/TupleExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -920,7 +920,7 @@ public static Tuple<T1, T2, T3, T4, T5, T6, T7, Tuple<T8, T9, T10, T11, T12, T13
}
#endregion

private static ValueTuple<T1, T2, T3, T4, T5, T6, T7, TRest> CreateLong<T1, T2, T3, T4, T5, T6, T7, TRest>(T1 item1, T2 item2, T3 item3, T4 item4, T5 item5, T6 item6, T7 item7, TRest rest) where TRest : struct =>
private static ValueTuple<T1, T2, T3, T4, T5, T6, T7, TRest> CreateLong<T1, T2, T3, T4, T5, T6, T7, TRest>(T1 item1, T2 item2, T3 item3, T4 item4, T5 item5, T6 item6, T7 item7, TRest rest) where TRest : struct, ITuple =>
new ValueTuple<T1, T2, T3, T4, T5, T6, T7, TRest>(item1, item2, item3, item4, item5, item6, item7, rest);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need the constraint to struct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TRest must be a nested ValueTuple.
Unfortunately, we don't have a good way to express that constraint, so I am using a weaker (but correct) constraint. ValueTuple is a struct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why ITuple is not enough of a constraint?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ITuple is public (other types can implement it). It applies to any type that can be used in dynamic pattern matching (when you want to break an object into its elements at runtime).
Also ITuple says nothing about class vs struct, but ValueTuple must be a struct to be usable by the compiler (this will be implemented shortly, dotnet/roslyn#11689).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We seem to completed a cycle: I asked why it has to be a struct, and now you said that [because] "ValueTuple must be a struct". So, again, why does it have to be a struct?

Copy link
Member

@KrzysztofCwalina KrzysztofCwalina Jul 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I love the fact that now tuples (ValueTyple*) are structs. I just think that constraining TRest to struct is unnecessary. If TRest is generated by the compiler, you will make it a struct, i.e. you don't need the constraint.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion on the struct constraint, but note that removing it will likely force us to add a bit more code, e.g. to protect against Rest being null here:
https://github.com/dotnet/corefx/blob/master/src/System.ValueTuple/src/System/ValueTuple/ValueTuple.cs#L2042
Since we really want this to be constrained to be a value tuple, all of which are structs, and we don't expect nor want anything else in this position, why remove the constraint? What harm does it cause to have it (i.e. what scenario are you envisioning)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, I was simply asking why we have the constraint, not trying to push to remove it. And so I gather the answer is that we don't want to do null checks in ToString?

But secondly, I wonder if it would not be better for rest to be a class after a certain arity. I think past certain arity, the struct will get so large that it would be cheaper to allocate on the heap than to make the copies.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not the reason; I was simply pointing out a side effect of removing it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me, the reason is that we want to constrain to a ValueTuple, but we can't express that, so we apply the maximum constrain that we can express.

I find the idea of switching to nesting class (most likely System.Tuple) after a certain arity intriguing. But that means the compiler would have to be aware of that type too. Even if we removed the struct constraint to leave room for such an option in the future, we'd face the problem that ValueTuple and System.Tuple don't implement the same internal interface.


private static Tuple<T1, T2, T3, T4, T5, T6, T7, TRest> CreateLongRef<T1, T2, T3, T4, T5, T6, T7, TRest>(T1 item1, T2 item2, T3 item3, T4 item4, T5 item5, T6 item6, T7 item7, TRest rest) =>
Expand Down
Loading