Add std.range.tail #3855

JakobOvrum · 2015-12-05T17:20:21Z

Forum thread:
http://forum.dlang.org/post/n3sfgg$1hpm$3@digitalmars.com

Differences from Andrei's suggestion:

Returns Range, not TakeExactly!Range - the end of the resulting range is always the same as the input so this checks out and is a useful artifact.
Supports non-forward ranges when hasLength.
Rejects infinite ranges.

I thought tail was clever so I named it that, but I'll happily change it to the originally proposed advanceWithin if people want that.

JackStouffer · 2015-12-05T17:51:07Z

std/range/package.d

+{
+    static immutable input = [1, 2, 3];
+    static immutable expectedOutput = [2, 3];
+    assert(input.tail(2) == expectedOutput);


static assert?

It's for checking @nogc, so no

You can do both at the same time, no?

What do you mean?

If you change it to a static assert, you can test CTFE and @nogc at the same time, right?

Compile-time evaluated expressions are not restricted by the attributes of the surrounding function.

When it comes to testing CTFE, this isn't generally done for most of our library. To wit, the only function in std.range.package that tests CTFE appears to be takeNone. This isn't entirely unreasonable because CTFE restrictions aren't really relevant in high level code.

However, issues do crop up when optimizations are applied, such as for array and find. In this case it's obvious that no CTFE restrictions are violated, but perhaps in some strange future this algorithm too could be optimized with some kind of low level trick. When testing CTFE though, it's not enough to test just one application - not only will different input types produce different functions, but CTFE restrictions only apply when they are encountered in the path currently being executed.

In this particular case CTFE can be tested decently well by just adding a single line to the static foreach, so I did that. It's not that simple in general.

JakobOvrum · 2015-12-05T18:32:00Z

@MetaLang, I missed your comment, but the answer is probably: checking both for empty and comparing length with n is probably more work than necessary in most cases. It is assuming that empty is not significantly cheaper than length. Also, most calls are probably on non-empty ranges? BTW, I've found that deleting a comment can really confuse email users!

MetaLang · 2015-12-05T18:34:33Z

Yes, I realized this just after I posted the comment so I deleted it.

JackStouffer · 2015-12-05T23:00:59Z

LGTM.

I like tail better than advanceWithin; instantly makes sense to posix users.

andralex · 2015-12-06T02:34:38Z

The idea behind TakeExactly is to provide extra information to the user. The length of the result is already computed so it's more informative to make it part of the result. Otherwise that information is lost.

andralex · 2015-12-06T02:36:40Z

std/range/package.d

+    }
+    else
+    {
+        if (range.empty)


No need for this - just move scout.popFront() to the end of the loop below.

It means more calls to save when the length of range is 0 or n + 1, but I suppose there's no reason to optimize for these cases.

JakobOvrum · 2015-12-06T12:10:43Z

The current implementation is cute, but is it any different from the following?

immutable length = range.walkLength; // note: O(1) when range has length
auto tail = range.drop(n > length? length - n : 0); // note: O(1) when range has slicing

quickfur · 2015-12-09T19:33:31Z

std/range/package.d

+        }
+
+        auto tail = range.save;
+        while(!scout.empty)


Nitpick: space between while and (.

quickfur · 2015-12-09T19:36:02Z

Other than that, LGTM.

Suggested by Andrei Alexandrescu in this thread: http://forum.dlang.org/post/n3sfgg$1hpm$3@digitalmars.com

JakobOvrum · 2015-12-09T20:25:06Z

Fixed.

Reimplementing this in user code requires at least one variable which makes it tedious for range composition, so even if the implementation offers no significant advantage in terms of performance or range capabilities (both input and output), I think it's worth including. Further, I think the branch for !hasLength!Range in the current implementation might offer some marginal improvement in edge cases (for one it's not limited by size_t unlike drop) and it's nice and short, so I think it's probably fine as-is.

JakobOvrum · 2015-12-11T06:40:38Z

std/range/package.d

+            .lineSplitter
+            .tail(2)
+            .joiner("\n")
+            .equal("two\nthree"));


Just now in a script I could have used just this pattern (lineSplitter + tail) to parse some output from a child process. Neat.

JakobOvrum · 2015-12-12T00:50:47Z

I think I should add a path for bidirectional ranges with length.

What about bidirectional ranges without length, though? Should we assume the beginning of the tail is closer to the end than the beginning?

JackStouffer · 2015-12-12T02:10:04Z

Should we assume the beginning of the tail is closer to the end than the beginning?

I'd say that that's a pretty safe assumption.

JakobOvrum · 2015-12-12T02:21:34Z

When n is more or less constant, the beginning of the tail moves proportionally closer to the end of the range as the number of elements in the range grows. That's pretty good for iterating from the back.

If n is calculated with the length of the range, we can experience the opposite, which is not good. However, this is probably a mistaken use of tail - stuff like drop should be used instead.

When n is not constant but calculated unrelated to the length of the range, then it cannot be known whether iterating from the front or iterating from the back is better.

So I guess iterating from the back is the slightly better choice overall.

JakobOvrum · 2015-12-12T02:27:55Z

Erm, iterating from the back is not readily possible. Not sure what I was thinking.

JackStouffer · 2015-12-13T00:21:54Z

Erm, iterating from the back is not readily possible. Not sure what I was thinking.

If you add the ability to make n a template argument you could.

JakobOvrum · 2015-12-13T00:23:32Z

How so?

JackStouffer · 2015-12-13T00:26:55Z

You could make a static array as a buffer ElementType!Range[n] buff for what ever you get off the end.

JakobOvrum · 2015-12-13T01:43:31Z

E[n] buffer;
auto remainder = range.retro.take(n).copy(buffer[].retro);
auto result = buffer[remainder.length .. $];

Whether or not this is appropriate for a standard library function aside, I think it's inappropriate for tail for a number of reasons, primarily because the return type would have to work like only and carry all its elements in-place, which is fundamentally different from what tail does now.

JackStouffer · 2016-01-06T14:10:39Z

@quickfur @andralex This looks good to go from people's comments.

quickfur · 2016-01-06T19:46:59Z

ping @andralex

Is this OK to merge? Are the objections you raised about using TakeExactly still blocking this PR, or the explanation is good enough?

JakobOvrum · 2016-01-07T03:35:52Z

Oh, I forgot to mention that the implementation does use takeExactly now. I think there are benefits to either way.

I haven't linked this to an issue report yet. (edit: or would a manual changelog entry be preferable?)

JackStouffer · 2016-01-07T03:48:23Z

would a manual changelog entry be preferable?

I think so

andralex · 2016-01-12T05:47:36Z

Auto-merge toggled on

Add std.range.tail

nordlow · 2016-01-13T13:40:59Z

For symmetry I truly feel that front() now should have an extra optional size_t n argument aswell. What do you say?

JakobOvrum · 2016-01-13T13:43:40Z

@nordlow, what do you mean? The front version of tail(n) is take(n).

andralex · 2016-01-13T13:48:19Z

@nordlow that won't work. Some ranges define front property-style, with a getter and setter overloads.

nordlow · 2016-01-13T13:56:45Z

@andralex Ok.

JakobOvrum force-pushed the range_tail branch 5 times, most recently from 780bdc5 to d856f3a Compare December 5, 2015 17:41

JackStouffer reviewed Dec 5, 2015
View reviewed changes

JakobOvrum force-pushed the range_tail branch from d856f3a to d7950e9 Compare December 5, 2015 19:59

andralex reviewed Dec 6, 2015
View reviewed changes

JakobOvrum force-pushed the range_tail branch from d7950e9 to 374a75c Compare December 6, 2015 11:51

JakobOvrum force-pushed the range_tail branch 2 times, most recently from 673857a to 76cb935 Compare December 7, 2015 20:09

quickfur reviewed Dec 9, 2015
View reviewed changes

std/range/package.d

}

auto tail = range.save;

while(!scout.empty)

Copy link

Member

quickfur Dec 9, 2015

Choose a reason for hiding this comment

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

Nitpick: space between while and (.

Add std.range.tail

a16cfc0

Suggested by Andrei Alexandrescu in this thread: http://forum.dlang.org/post/n3sfgg$1hpm$3@digitalmars.com

JakobOvrum force-pushed the range_tail branch from 76cb935 to a16cfc0 Compare December 9, 2015 19:59

JakobOvrum reviewed Dec 11, 2015
View reviewed changes

andralex added a commit that referenced this pull request Jan 12, 2016

Merge pull request #3855 from JakobOvrum/range_tail

1e74490

Add std.range.tail

andralex merged commit 1e74490 into dlang:master Jan 12, 2016

Uh oh!

Add std.range.tail #3855

Add std.range.tail #3855

Uh oh!

Conversation

JakobOvrum commented Dec 5, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JakobOvrum commented Dec 5, 2015

Uh oh!

MetaLang commented Dec 5, 2015

Uh oh!

JackStouffer commented Dec 5, 2015

Uh oh!

andralex commented Dec 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JakobOvrum commented Dec 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quickfur commented Dec 9, 2015

Uh oh!

JakobOvrum commented Dec 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JakobOvrum commented Dec 12, 2015

Uh oh!

JackStouffer commented Dec 12, 2015

Uh oh!

JakobOvrum commented Dec 12, 2015

Uh oh!

JakobOvrum commented Dec 12, 2015

Uh oh!

JackStouffer commented Dec 13, 2015

Uh oh!

JakobOvrum commented Dec 13, 2015

Uh oh!

JackStouffer commented Dec 13, 2015

Uh oh!

JakobOvrum commented Dec 13, 2015

Uh oh!

JackStouffer commented Jan 6, 2016

Uh oh!

quickfur commented Jan 6, 2016

Uh oh!

JakobOvrum commented Jan 7, 2016

Uh oh!

JackStouffer commented Jan 7, 2016

Uh oh!

andralex commented Jan 12, 2016

Uh oh!

nordlow commented Jan 13, 2016

Uh oh!

JakobOvrum commented Jan 13, 2016

Uh oh!

andralex commented Jan 13, 2016

Uh oh!

nordlow commented Jan 13, 2016

Uh oh!

Uh oh!