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.

Sign up for GitHub

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

Jump to bottom

Generator implements InputRange interface #5515

Merged
merged 2 commits into from
Jul 1, 2017
Merged

Generator implements InputRange interface #5515

merged 2 commits into from
Jul 1, 2017

Conversation

dukc
Copy link
Contributor

@dukc dukc commented Jun 26, 2017

Reboot of #5309 which is a reboot of #5194.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @dukc! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

  • smaller, focused PRs are easier to review than big ones

  • try not to mix up refactoring or style changes with bug fixes or feature enhancements

  • provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

@dukc dukc mentioned this pull request Jun 26, 2017
Closed
@dukc dukc force-pushed the fiberRangeInterface branch from 18eeb7f to 5656d9b Compare June 26, 2017 20:48
@dukc
Since std.concurrency.Generator is a class already, I made it to impl…
5983bcc 
…ement

std.range.interfaces.InputRange without having to call inputRangeObject().
@dukc dukc force-pushed the fiberRangeInterface branch from 5656d9b to 5983bcc Compare June 26, 2017 20:50
adamdruppe
adamdruppe approved these changes Jun 27, 2017
View reviewed changes
Copy link
Contributor

@adamdruppe adamdruppe left a comment

Choose a reason for hiding this comment

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

It looks OK to my.

MetaLang
MetaLang requested changes Jun 27, 2017
View reviewed changes
Copy link
Member

@MetaLang MetaLang left a comment

Choose a reason for hiding this comment

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

This is good, but I'd suggest adding a few more tests (preferably non-trivial ones). Maybe look at some of the std.range tests for inspiration.

std/concurrency.d Outdated
@@ -74,6 +74,14 @@ import std.traits;

private
{
import core.atomic;
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove the un-needed imports? (core.thread, core.sync.mutex, core.sync.condition, std.range.primitives)

std/concurrency.d Outdated
import core.sync.condition;
import std.range.primitives;
import std.range.interfaces;
import std.traits;
Copy link
Member

Choose a reason for hiding this comment

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

import std.traits: hasElaborateCopyConstructor

std/concurrency.d Outdated
@@ -1624,6 +1672,7 @@ void yield(T)(T value)
{
import core.exception;
import std.exception;
import std.range.interfaces;
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this import.

std/concurrency.d Outdated
@@ -1661,14 +1710,49 @@ void yield(T)(T value)
{
tid.send(e);
}

Copy link
Member

Choose a reason for hiding this comment

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

This isn't a show stopper but there's no reason to clutter the diff with extraneous whitespace.

std/concurrency.d Outdated

// MessageBox Implementation
Copy link
Member

Choose a reason for hiding this comment

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

Is this related to the changes in question? If not please remove it.

drug007
drug007 reviewed Jun 27, 2017
View reviewed changes
std/concurrency.d Outdated
*/
final T front() @property
{
return *m_value;
}

/**
* Returns the most recently generated value without excuting a copy
Copy link
Contributor

Choose a reason for hiding this comment

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

typo executing

MetaLang
MetaLang reviewed Jun 27, 2017
View reviewed changes
std/concurrency.d Outdated
@@ -1615,7 +1608,7 @@ class Generator(T) :
else
{
static assert(0,
"Fiber front is rvalue and thus cannot be moved when it defines a postblit.");
"Fiber front is always rvalue and thus cannot be moved if 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.

We know here that T defines an elaborate copy constructor so it might be better to mention it by name. Better wording for this message might be:

"Fiber front cannot be moved because " ~ t.stringof ~ " defines a postblit"

Another option is to use a template guard instead, like:

final T moveFront()
if (!hasElaborateCopyConstructor!T)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Template guard won't do since then postblitting fibers would not implement InputRange anymore. I can clarify wording trough.

@dukc dukc force-pushed the fiberRangeInterface branch from 71793fa to 13fc6cf Compare June 27, 2017 14:28
@MetaLang
Copy link
Member

@wilzbach why hasn't the bot suggested someone from the core team that could review this PMR? Has that functionality been disabled?

@MetaLang MetaLang mentioned this pull request Jun 29, 2017
Merged
@wilzbach
Copy link
Member

@wilzbach why hasn't the bot suggested someone from the core team that could review this PMR? Has that functionality been disabled?

Yes, we disabled it because it was too noisy. IIRC it was only enabled for one week in August 2016: dlang-bots/mention-bot#7
(that feature was never part of the bot's source code, but of Facebook's mention-bot).

Btw (for contributors) GH now suggests reviewers based on the diff and other information:

image

JackStouffer
JackStouffer approved these changes Jun 29, 2017
View reviewed changes
Copy link
Member

@JackStouffer JackStouffer left a comment

Choose a reason for hiding this comment

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

Everything looks ok to me.

@wilzbach wilzbach force-pushed the fiberRangeInterface branch from 13fc6cf to 942da35 Compare July 1, 2017 00:10
@wilzbach
Copy link
Member

wilzbach commented Jul 1, 2017

This isn't a show stopper but there's no reason to clutter the diff with extraneous whitespace.

Removed the whitespace line from the diff.

@dlang-bot dlang-bot merged commit 83ae83c into dlang:master Jul 1, 2017
@MetaLang
Copy link
Member

MetaLang commented Jul 1, 2017 •
edited
Loading

Thanks @JackStouffer and @wilzbach. There's one other one that needs to be looked at when you have some time; it apparently fixes random segfaults in the std.concurrency test suite. Unfortunately I'm not familiar with std.concurrency so I can't review it.

#5004

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

@MetaLang MetaLang MetaLang requested changes

@drug007 drug007 drug007 left review comments

@JackStouffer JackStouffer JackStouffer approved these changes

@adamdruppe adamdruppe adamdruppe approved these changes

Assignees
No one assigned
Labels
Merge:auto-merge Severity:Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@dukc @dlang-bot @MetaLang @wilzbach @adamdruppe @JackStouffer @drug007