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

[WIP] Implement ruby-style Enumerator #6385

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

felixbuenemann
Copy link
Contributor

This is a work-in-progress and shouldn't be merged yet!

I'll update the description of the PR once the details are fully flushed out.

For discussion see #6357.

A big thanks to @asterite who helped in implementing most of the fiber stuff.

@RX14
Copy link
Contributor

RX14 commented Jul 14, 2018

If you turn @@stack_pool into another linked list of fibers (not their stacks, the fiber instances would be discarded in the case of stack reuse or deallocation), then you can skip the whole mark/sweep fiber collection to avoid allocating in finalize. I think it's a slightly cleaner solution given the pointers for a linked list are there already.

@RX14
Copy link
Contributor

RX14 commented Jul 14, 2018

Also, I'd really really rather not call it an enumerator. Given my java/C# experience Enumerator == Iterator. I'd name it Iterator::Fiber or Iterator::Yielding or something more descriptive than Enumerator.

@felixbuenemann
Copy link
Contributor Author

I agree that having both Iterator and Enumerator is very confusing and should be avoided.

I've spend quite some time thinking about this and I think it's the wrong level of abstraction.

  • Maybe the right abstraction is having a top level Yielder class that can be used to wrap a suspendable block of code and just having an Iterator.yielder(&block) convenience class method to construct a Yielder from a block and use it as an iterator, similar to Iterator.of.
  • Maybe this was a bad idea in the first place and should be scrapped.

I'll update #6357 with a more detailed analysis once I've fully thought it through.

@asterite
Copy link
Member

For me Enumerator is fine. It's also called like that in Ruby..

@felixbuenemann
Copy link
Contributor Author

Yes, but Ruby does not have an Iterator class.

@asterite
Copy link
Member

But Java does.
Iterator in Crystal is very similar to Java.
Enumerator in Crystal will be very similar to Ruby.
I personally think that's fine.

@RX14
Copy link
Contributor

RX14 commented Jul 15, 2018

@asterite but Enumerator in C# is very similar to Iterator in Crystal. That's the confusion. The naming doesn't really matter anyway, since we can probably make the interface just Iterator.new(&block) or Iterator.yielder(&block).

@asterite
Copy link
Member

That's what I thought, but no, because you always have to specify the generic type. But maybe Iterator::Something(T) could work.

I'm still not 100% convinced about the implementation, I feel like something is missing.

@felixbuenemann
Copy link
Contributor Author

I'm still not 100% convinced about the implementation, I feel like something is missing.

I'm not too happy about the changes to Fiber that the current implementation requires – it seems overkill. I think I'll explore an alternate approach of raising an exception inside the yielder block.

@felixbuenemann
Copy link
Contributor Author

Btw. I also noticed that we are currently lacking exception bubbling from the yielder block to the wrapping enumerator, because exceptions in fibers are silently swallowed.

I've already implemented that locally by rescuing from exceptions in the fiber block and assigning it to an instance variable on the enumerator. The exception will then be re-raised in #fetch_next.

I'll add it to the WIP implementation along with specs in the next iteration.

run
end

# Returns the next element yielded from the block or `Iterator::Stop::INSTANCE`
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is an Enumerator should the return type be an Enumerator::Stop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't tested it, but since the Iterator module is included it probably already works with both Enumerator::Stop and Iterator::Stop.

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.

None yet

5 participants