Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tuple range interface #3198

Closed
wants to merge 1 commit into from

Conversation

JakobOvrum
Copy link
Member

Adds a range interface for tuples with homogeneous types. The introduced range interface presents the fields by reference, so it's very different from only(myTuple.expand), as the examples show.

The whitespace fixes are in a separate commit, so the diff for the second commit is probably easier to look at than the total diff.

Could the new return attribute help with making this a memory safe interface?


Do tuples with internal padding exist? Or rather, do template argument lists with padding exist? I wasn't able to produce any with DMD32/Windows/OMF; I do have the code that handles it properly so it's readily available if padding is possible. Right now this patch includes a static assert checking that no tuple with padding is ever instantiated.


Apparently, documented unittests inside a documented static-if like this confuses DDoc quite severely, so the DDoc description is duplicated three times in the output. I'll see if the bug tracker has anything on this.

@JakobOvrum JakobOvrum force-pushed the tuple_range_interface branch 2 times, most recently from 1f1332b to 7a40ffa Compare April 18, 2015 12:51
/+ Helper for partial instanciation +/
template isBuildableFrom(U)
{
enum isBuildableFrom(T) = isBuildable!(T, U);
}

/+ Determine whether or not all given types in T are the same type +/
template areAllSame(T...)
Copy link
Member

Choose a reason for hiding this comment

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

Do you think there's any value in also supporting Tuples whose types can all implicitly convert to some supertype?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about this, but ultimately, I think it's better without. The biggest issue is that the element type wouldn't be an lvalue like it is with the current implementation.

@MetaLang
Copy link
Member

This is an interesting PR, but it suffers somewhat from the fact that the following code works:

import std.array: array;
import std.typecons: Tuple;

void main()
{
    Tuple!(int, int, int) intTup;
    int[] tupArr1 = intTup.array;
    int[] tupArr2 = [intTup.expand];
    //Curiously *doesn't* work
    //int[] tupArr3 = intTup.to!(int[]);
}

And there is also as an option, as you said, only(intTup.expand). All of these options have the benefit of being @safe as well. The only real advantage of Tuple.range is that it doesn't allocate, but safety is sacrificed to achieve this, and thus it won't be usable in large swathes of code.

@JakobOvrum
Copy link
Member Author

The only real advantage of Tuple.range is that it doesn't allocate

No. The real advantage is that the range it returns refers to the original elements by reference, allowing code like the examples.

/+ Determine whether or not all given types in T are the same type +/
template areAllSame(T...)
{
static if (T.length == 0 || T.length == 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Otherwise known as T.length <= 1 ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is more explicit, i.e. "match the nullary and unary cases".

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 more likely to raise a brow with most maintainers than the simpler condition, so please change and let's merge this. I approve the PR and will archive this.

@JakobOvrum JakobOvrum force-pushed the tuple_range_interface branch from 7a40ffa to bf9c047 Compare April 28, 2015 05:56
@JakobOvrum
Copy link
Member Author

areAllSame now uses allSatisfy for better recursion performance. We should have a template so that we could do Types.length && allSatisfy!(isExactly!(Types[0]), Types) right there in the template constraint for range, but that's beyond the scope of this PR, hence why this patch has areAllSame to begin with.

@JakobOvrum JakobOvrum force-pushed the tuple_range_interface branch from bf9c047 to 1cdca5e Compare October 11, 2015 01:45
@andralex
Copy link
Member

This is fine. In the future we may expand functionality to the empty tuple and tuples that contain qualified types with different qualifiers etc.

@JakobOvrum JakobOvrum force-pushed the tuple_range_interface branch from 1cdca5e to 86de84c Compare December 30, 2015 12:09
@JakobOvrum
Copy link
Member Author

Fixed the awkward condition and rebased.

@JakobOvrum JakobOvrum force-pushed the tuple_range_interface branch 2 times, most recently from f411658 to 2283d5d Compare December 30, 2015 12:58
@JakobOvrum
Copy link
Member Author

Amended it to handle tuples containing types of varying mutability. For nullary only() we made the element type int - should I go ahead and do the same for the empty tuple?

@JakobOvrum JakobOvrum force-pushed the tuple_range_interface branch from 2283d5d to d318811 Compare January 7, 2016 21:06
@dlang-bot
Copy link
Contributor

dlang-bot commented Jan 7, 2016

Fix Bugzilla Description
5489 std.typecons tuples dynamically iterable
9871 std.typecons.asArray

@JakobOvrum JakobOvrum force-pushed the tuple_range_interface branch from d318811 to 6d89732 Compare January 7, 2016 21:08
}

///
nothrow unittest
Copy link
Member

Choose a reason for hiding this comment

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

If you put the unittest inside the struct, there will be a new unittest instantiated each time Tuple is instantiated. Jonathan Davis pointed this out in the newsgroup and I think it's something we should avoid.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, but it should be done for the whole type. I pointed out the same in #3594.

Copy link
Member

Choose a reason for hiding this comment

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

Ndslice does this a lot, too.
AFAIK this works as long as the are enough outside tests covering all types.
See #4050

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a performance issue: code is generated and executed an unnecessary amount of times. I haven't heard of any measurements indicating that Tuple in particular is a problem but it's good to avoid in principle.

Copy link
Member

Choose a reason for hiding this comment

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

how about adding version (unittest){}?

@JakobOvrum JakobOvrum mentioned this pull request Feb 5, 2016
@wilzbach
Copy link
Member

wilzbach commented Mar 6, 2016

I have suffered from the bad interoperability between tuples and arrays a lot since I started with D and I would like to see this coming too :)

If I understood this PR, this we can do this in both ways?

Tuple!(int, int) tup = tuple(1, 2);
auto arr1 = tup.range; 
assert(tup == arr1);

// and back again
auto arr2 = [3, 4]
tuple.range = arr2; 
assert(tup == arr2);

Would be pretty useful!

@JakobOvrum
Copy link
Member Author

Neither of the two operations you posted would work with this. There is a distinction between arrays and ranges, and this PR adds a range interface, not an array interface, to tuples.

Here's how you do those operations in range style:

Tuple!(int, int) tup = tuple(1, 2);
int[] arr1 = tup.range.array; // Assuming you meant making a copy, as otherwise you'd just compare the slice with itself
assert(tup.range == arr1);

// and back again
int[] arr2 = [3, 4];
arr2.copy(tuple.range);
assert(tup.range == arr2);

@wilzbach
Copy link
Member

wilzbach commented Mar 6, 2016

Neither of the two operations you posted would work with this. There is a distinction between arrays and ranges, and this PR adds a range interface, not an array interface, to tuples.

What do you think about adding this example as another visible unittest in the documentation?

Fix ticket 5489 and 9871
@JakobOvrum JakobOvrum force-pushed the tuple_range_interface branch from 6d89732 to 2697b9e Compare March 14, 2016 09:19
@wilzbach
Copy link
Member

@JakobOvrum now autotester fails ...

@DmitryOlshansky
Copy link
Member

@JakobOvrum now autotester fails ...

OMG looks like compiler internal error, all more valuable to reduce.

@wilzbach
Copy link
Member

OMG looks like compiler internal error, all more valuable to reduce.

Then let's bug it? Do you get from the error message what exactly fails? Is someone here using FreeBSD?

@JackStouffer
Copy link
Member

No activity from the author in nine months. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants