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

Generator implements InputRange #5194

Closed
wants to merge 4 commits into from
Closed

Conversation

dukc
Copy link
Contributor

@dukc dukc commented Feb 24, 2017

Since std.concurrency.Generator is a class already, I made it to implement std.range.interfaces.InputRange without having to call inputRangeObject().

@dukc
Copy link
Contributor Author

dukc commented Feb 24, 2017

Uh oh, it seems i need to replace that uint index with size_t...

…ement

std.range.interfaces.InputRange without having to call inputRangeObject().
@dukc
Copy link
Contributor Author

dukc commented Feb 24, 2017

Now seems to work also in 64bit.

@wilzbach
Copy link
Member

@dukc: mind the test failures:

Enforce whitespace before opening parenthesis
grep -nrE "(for|foreach|foreach_reverse|if|while|switch|catch)\(" $(find . -name '*.d') ; test $? -eq 1
./std/concurrency.d:1715:        for(; !empty; popFront())
./std/concurrency.d:1718:            if(broken) break;
./std/concurrency.d:1726:        for(size_t i; !empty; ++i, popFront())
./std/concurrency.d:1729:            if(broken) break;
./std/concurrency.d:1836:        foreach(i; 0 .. 10) yield(i);
make: *** [style] Error 1

Also you might want to consider to use the CodeCov extension (or click on the link, or run the coverage locally) as opApply is uncovered.

static assert(0,
"Fiber front is rvalue and thus cannot be moved when it defines a postblit.");
}
}
Copy link
Member

Choose a reason for hiding this comment

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

How is this different to the generic moveFront implementation?

    static if (is(typeof(&r.moveFront)))
    {
        return r.moveFront();
    }
    else static if (!hasElaborateCopyConstructor!(ElementType!R))
    {
        return r.front;
    }
    else static if (is(typeof(&(r.front())) == ElementType!R*))
    {
        import std.algorithm.mutation : move;
        return move(r.front);
    }
    else
    {
        static assert(0,
                "Cannot move front of a range with a postblit and an rvalue front.");
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Except for the message, no difference. I tried to call it, but the first static if caused infinite recursion.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@dukc dukc Mar 3, 2017

Choose a reason for hiding this comment

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

First off, we are talking about std.concurrency.Generator, not about std.range.generate. But that code would work with Generator too, in DPaste.

The reason it does not work in Generator: Look the first static if block in moveFront implementation you just posted. MoveFront(generator) calls generator.moveFront if it can find one -the very function I'm defining!

return broken;
}

final int opApply(scope int delegate(size_t, T) loopBody)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm if we have to add opApply to everything that is an InputRange the range concept of "building block" would have somehow failed. Do you know about enumerate and why doesn't it work for you?

Copy link
Contributor Author

@dukc dukc Feb 27, 2017

Choose a reason for hiding this comment

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

I did not think about it. But wouldn't that plant a land mine for maintenance? If somebody adds an opApply implementation using foreach to enumerate(), it would regress Generators opApply because it would recurse infinitely then.

Copy link
Contributor Author

@dukc dukc Feb 27, 2017

Choose a reason for hiding this comment

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

What we could add to avoid code duplication for cases like this, is a template mixin version of opApply() and moveFront().

Edit: Or a generic range template which guarantees in it's spec to forward only primitives and nothing else.

Copy link
Member

Choose a reason for hiding this comment

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

If somebody adds an opApply implementation using foreach to enumerate(), it would regress Generators opApply because it would recurse infinitely then.

I am sorry, but I can't follow. How would sth. like this half-pseudo code end in an infinite recursion?

struct Enumerate(R)
{
    R r;
    int opApply(scope int delegate(size_t, ElementType!R) dg)
    {
         int err;
         auto r2 = r.save;
         for (size_t i; !r2.empty; r2.popFront(), i++)
         {
             err = dg(i, r2.front);
             if (err) return err;
         }
         return 0;
    }
}
myGenerator.enumerate.array;

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@dukc dukc Mar 1, 2017

Choose a reason for hiding this comment

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

That code won't. But if it's done like (untested, may have errors):

int opApply(scope int delegate(size_t, ElementType!R) dg)
{
         int err;
        static if(__traits(compiles, foreach(i,e;r){})) foreach (i, e; r.save)
        {
              err = dg(i, e);
             if (err) return err;
         } else for (size_t i; !r2.empty; r2.popFront(), i++)
         {
             auto r2 = r.save;
             err = dg(i, r2.front);
             if (err) return err;
          }
         return 0;
}

That, I think, will, because the foreach inside calls the very indexed opApply of R what's using the Enumerate in it's implementation.

@@ -1586,8 +1586,9 @@ private interface IsGenerator {}
* }
* ---
*/
import std.range.interfaces : InputRange;
Copy link
Member

Choose a reason for hiding this comment

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

goes at the top of the file

Copy link
Contributor Author

@dukc dukc Feb 27, 2017

Choose a reason for hiding this comment

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

Doesn't: there was one already! I removed mine altogether.

EDIT: I read that import wrong. Went messy so i squashed.

Removed another useless import.

One import wasn't reduntant after all...
@dukc
Copy link
Contributor Author

dukc commented Feb 27, 2017

I have no idea what those Linux failures are about... Does somebody have ideas?

EDIT: and now it claims all have passed, even when I did no change!?

@wilzbach
Copy link
Member

I have no idea what those Linux failures are about... Does somebody have ideas?

-> http://forum.dlang.org/post/bqgywgewtblzpeynrnxu@forum.dlang.org

You can click on "Deprecate" for specific builds, but the auto-tester will also perodically iterate through the entire queue and retest your PR ;-)

@dukc
Copy link
Contributor Author

dukc commented Mar 3, 2017

@wilzbach @JackStouffer
CircleCI appears, as of writing, to be stuck in it's execution. I've checked back many times, never seeing it completed.

Otherwise, ready for further discussion.

@wilzbach
Copy link
Member

wilzbach commented Mar 3, 2017

CircleCI appears, as of writing, to be stuck in it's execution. I've checked back many times, never seeing it completed.

Yeah I have observed this a couple of times already since the S3 outage this week. Don't worry about it.

@dukc
Copy link
Contributor Author

dukc commented Mar 8, 2017

@wilzbach I have answered your concerns of the code 5 days ago, let's continue discussion?

@JackStouffer
Copy link
Member

@dukc people get around to PRs when they can

Seb is currently off the grid and probably will be for the rest of the week.

@dukc
Copy link
Contributor Author

dukc commented Mar 8, 2017

Ok. Just wanted to make sure this wasn't forgotten.

@dukc
Copy link
Contributor Author

dukc commented Mar 28, 2017

This got too old so i close. I will probably reboot it.

@dukc
Copy link
Contributor Author

dukc commented Apr 27, 2017

link to new pr: #5309

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

Successfully merging this pull request may close these issues.

3 participants