Issue 9882 - Implement a "tee" style InputRange so that a function can be called during a chain of InputRanges. #1348

Closed
wants to merge 11 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

irritate commented Jun 15, 2013

Adds a TeeRange struct, and a tee template for creating the range as part of a chain of InputRanges.

This implements part of the requested enhancement behavior from Issue 9882 (http://d.puremagic.com/issues/show_bug.cgi?id=9882), to be able to insert debug writelns into a UFCS chain. Though the syntax and actual output will be slightly different than what is discussed in that issue (e.g. writelns are done on a per-element basis, instead of printing the entire range at once).

However, this solution is more generic, and can be used for other things. It's basically the "tap" part of the issue discussion, but I used the name "tee" since that is the well-known Unix name for piping behavior similar to this.

@monarchdodra monarchdodra commented on an outdated diff Jun 15, 2013

+ }
+
+ @property auto ref front()
+ {
+ if (!pipeOnPop)
+ {
+ func(_input.front);
+ }
+ return _input.front;
+ }
+
+ static if (isForwardRange!Range)
+ {
+ @property auto save()
+ {
+ return typeof(this)(_input);
@monarchdodra

monarchdodra Jun 15, 2013

Collaborator

Call save, or you aren't actually saving.

return typeof(this)(_input.save);

@monarchdodra monarchdodra commented on an outdated diff Jun 15, 2013

+ static if (isInfinite!Range)
+ {
+ enum bool empty = false;
+ }
+ else
+ {
+ @property bool empty() { return _input.empty; }
+ }
+
+ void popFront()
+ {
+ if (pipeOnPop && !_input.empty)
+ {
+ func(_input.front);
+ }
+ _input.popFront();
@monarchdodra

monarchdodra Jun 15, 2013

Collaborator

assert that !_input.empty. There is no reason to actually run this test in release. What's more, the way you are doing it just silently avoids the problem:

//start with the check
assert(!_input.empty);
 /* or */
version(assert)
    if (_input.empty)
        throw new RangeException;

//Then do only a static if
static if (pipeOnPop)  
{  
    func(_input.front);  
}

//Finally, pop  
_input.popFront();  

Also, static if

@monarchdodra monarchdodra commented on an outdated diff Jun 15, 2013

+ {
+ @property bool empty() { return _input.empty; }
+ }
+
+ void popFront()
+ {
+ if (pipeOnPop && !_input.empty)
+ {
+ func(_input.front);
+ }
+ _input.popFront();
+ }
+
+ @property auto ref front()
+ {
+ if (!pipeOnPop)
@monarchdodra

monarchdodra Jun 15, 2013

Collaborator

static

static if (!pipeOnPop)

Collaborator

monarchdodra commented Jun 15, 2013

I like the pipeOnPop concept. The pull seems complete as a whole, and looks mostly good to me.

Could you present your approach in this thread:
http://forum.dlang.org/thread/ovbjcmogezbvsxrwfcol@forum.dlang.org
As there has been discussion on the subject outside of the bug report. But I think what you provided is exactly what we asked for, though I'm not sure it's what we wanted ;)

I'd also be tempted to ask for a std.typecons.Flag for the popOnFront parameter. As they say, phobos should eat its dogfood:

r.tee!(writeln(a), Yes.popOnFront);

Contributor

irritate commented Jun 16, 2013

Thanks for the feedback. I have addressed the noted issues. I'll also take a look at std.typecons.Flag, since I'm not already familiar with it.

@monarchdodra monarchdodra commented on an outdated diff Jun 16, 2013

+ auto tee(Range)(Range inputRange) if (isInputRange!(Range))
+ {
+ return TeeRange!(Range, unaryFun!func, pipeOnPop)(inputRange);
+ }
+}
+
+private struct TeeRange(Range, alias func, bool pipeOnPop)
+{
+ Range _input;
+
+ this(Range r)
+ {
+ _input = r;
+ }
+
+ auto opSlice() { return _input; }
@monarchdodra

monarchdodra Jun 16, 2013

Collaborator

opSlice(/*nothing*/) is not a range primitive. And when provided, at the very least, it should return typeof(this) or Unqual!(typeof(this)).

Here, you are returning the inner structure. That's strange. I'd get rid of it completely.

@monarchdodra monarchdodra and 1 other commented on an outdated diff Jun 16, 2013

+ // post-map: 1
+ // pre-map: 9
+---
+
+*/
+template tee(alias func, bool pipeOnPop = true) if (is(typeof(unaryFun!func)))
+{
+ auto tee(Range)(Range inputRange) if (isInputRange!(Range))
+ {
+ return TeeRange!(Range, unaryFun!func, pipeOnPop)(inputRange);
+ }
+}
+
+private struct TeeRange(Range, alias func, bool pipeOnPop)
+{
+ Range _input;
@monarchdodra

monarchdodra Jun 16, 2013

Collaborator

I think there was a push for all "underlying" ranges to be made public, and named source (just like for retro). so this way, a user could (not that I know a use case for it), write myTee.source.

@andralex : is there a status on this?

In any case, I'd say either make it private, or if it stays public, name it source.

@jmdavis

jmdavis Jun 16, 2013

Member

I think there was a push for all "underlying" ranges to be made public, and named source (just like for retro). so this way, a user could (not that I know a use case for it), write myTee.source.

It has been brought before, but I don't think that it's ever been discussed in depth. The main problem with it is that it completely bypasses the range. For instance, if you did it with filter, then what's in source wouldn't necessarily have much relation to what filter was iterating over, because filter skips at least some of the elements. I'd argue that no new ranges should make their source public without it being thoroughly discussed in the newsgroup first.

The thing that's off here is TeeRange isn't a Voldemort type, which is almost always how we do it now. There isn't much reason to declare TeeRange outside of tee if it's private.

Collaborator

monarchdodra commented Jun 16, 2013

I'll also take a look at std.typecons.Flag, since I'm not already familiar with it.

I was really just throwing that out there. AFAIK, it is not widespread at all. If you aren't comfortable with it, or disagree, feel free to say so.

Contributor

irritate commented Jun 16, 2013

Thanks again for the feedback. More concerns have been addressed.

I did run into one issue, however. I tried to make the TeeRange a Voldemort type by moving the struct into the generator function, and I hit the following error message during compilation for "cannot get frame pointer":

std\range.d(8915): Error: function test.__unittestL7_23.tee!(__lambda8).tee!(Fil
terResult!(__lambda6, int[])).tee.TeeRange!(FilterResult!(__lambda6, int[])).Tee
Range.popFront cannot get frame pointer to __lambda8

No other error messages occurred to give me a clue to the problem, so I am still trying to determine if this is something I am doing wrong, or if it's a compiler bug that I can create a simpler reproducing example for.

Collaborator

monarchdodra commented Jun 16, 2013

I did run into one issue, however

Did you declare the struct as static ? That's most probably it. Nested structs will have a hidden frame pointer, unless you declare it as static.

Also, if you do declare it inside tee, you might as well just call the struct Result (or Range)...

Contributor

irritate commented Jun 16, 2013

Yes, that's exactly what happened. I was educated by the "Nested Structs" section here http://dlang.org/struct.html.

Thanks.

irritate added some commits Jun 16, 2013

Contributor

irritate commented Jun 16, 2013

The automated tester failed in druntime in a core.thread assert:
core.exception.AssertError@core.thread(2557): unittest failure

As far as I can tell, that's unrelated to my last commit. But please let me know if I need to do anything for this.

Thanks.

Collaborator

monarchdodra commented Jun 16, 2013

The automated tester failed in druntime in a core.thread assert:

It's unrelated.

@ghost Unknown commented on the diff Oct 24, 2013

std/range.d
+ .tee!(a => writefln("pre-map: %d", a))
+ .map!(a => a + 1)
+ .tee!(a => writefln("post-map: %d", a))
+ .filter!(a => a < 10);
+ assert(equal(printValues, [2, 5]));
+ // Outputs (order due to range evaluation):
+ // post-map: 2
+ // pre-map: 1
+ // post-map: 5
+ // pre-map: 4
+ // post-map: 10
+ // pre-map: 9
+---
+
+*/
+template tee(alias func, Flag!"pipeOnFront" pipeOnFront = No.pipeOnFront)
@ghost

ghost Oct 24, 2013

I'd prefer a simple enum to Flag. It hasn't been used at all in Phobos and it shows why.

@ghost

ghost commented Feb 15, 2014

If OP wants to work on this again ping us and we'll reopen. Otherwise maybe someone else wants to continue the work?

ghost closed this Feb 15, 2014

Contributor

MetaLang commented Feb 22, 2014

I'd like to see this in Phobos. Is there anything else that needs to change for this to be accepted? If so, I'll fix it up and make a separate pull.

@ghost

ghost commented Feb 24, 2014

I don't like the name tee, it's not obvious at all what it does given its name. Otherwise it might be useful to have in Phobos.

Contributor

MetaLang commented Feb 24, 2014

I don't like the name either, but from the thread on the newsgroups I recall Andrei being partial to it. Off the top of my head, I can't think of anything better, either. Anyway, I will make my pull, and it can be discussed there.

@MetaLang MetaLang added a commit to MetaLang/phobos that referenced this pull request Feb 24, 2014

@MetaLang MetaLang Import std.functional.unaryFun, which got accidentally removed during…
… the cherry-pick

Import std.functional.unaryFun, which got accidentally removed during the cherry-pick

Revive dlang#1348 as it has been abondoned by the author.
5971c5f

@MetaLang MetaLang added a commit to MetaLang/phobos that referenced this pull request Feb 24, 2014

@MetaLang MetaLang Revive dlang#1348 as the author seems to have abandoned it. 51b34fc

@MetaLang MetaLang added a commit to MetaLang/phobos that referenced this pull request Feb 25, 2014

@irritate @MetaLang irritate + MetaLang Revive #1348: "Issue 9882 - Implement a "tee" style InputRange so tha…
…t a function can be called during a chain of InputRanges."
45c4e51
Owner

andralex commented Mar 16, 2014

FWIW I like it.

Contributor

MetaLang commented Mar 16, 2014

@andralex I closed my pull request #1348 to revive this due to objections from @blackwhale and @Poita. If you think this is something worth adding to Phobos, please comment and I'll reopen it.

Owner

andralex commented Mar 16, 2014

This is useful and powerful. Let's make this work as follows.

  1. tee takes a compile-time lambda or an output range. (For the lambda the equivalent to put would be just calling the lambda with the front element.)
  2. Whatever the upstream of tee is, tee offers an input range. No forward and no random. That takes care of all objections.
  3. tee writes to the other range ONLY upon the first call to front for each element. Essentially if the downstream range looks at an element, tee will record that in the teed range.

That's it. @Poita @blackwhale please destroy. Should work like a charm. @MetaLang please open a new pull request or use one of the existing ones.

Contributor

MetaLang commented Mar 17, 2014

@andralex Whoops, I linked the wrong pull. It's #1965 (and could you please duplicate your last message on that pull so I don't have to come back to this one?) I will reopen #1965 when I get a chance to work on it within the next week.

@Dicebot Dicebot added a commit that referenced this pull request Jul 16, 2014

@Dicebot Dicebot Merge pull request #1965 from MetaLang/std-range-tee-fixup
Revive pull request #1348: "Issue 9882 - Implement a "tee" style InputRange...
6d5ab30

This issue was closed.

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