Make `each` capable of early stopping by andralex · Pull Request #5268 · dlang/phobos

andralex · 2017-03-12T01:11:58Z

Flag to the rescue...

andralex · 2017-03-12T01:12:52Z

Code is a tad repetitive, ideas welcome on how to simplify.

WalterBright · 2017-03-12T01:14:11Z

std/algorithm/iteration.d

+
+Normally the entire range is iterated. If partial iteration (early stopping) is
+desired, `fun` needs to return a value of type $(REF Flag,
+std.typecons)`!"each"` (`No.each` to continue iteration, or `No.each` to stop


What happens if I specify Yes.each ?

WalterBright · 2017-03-12T01:14:49Z

std/algorithm/iteration.d


 Params:
    pred = predicate to apply to each element of the range
    r = range or iterable over which each iterates


quote each

WalterBright · 2017-03-12T01:15:40Z

std/algorithm/iteration.d

+iteration).

 Params:
    pred = predicate to apply to each element of the range


It's called fun, not pred

WalterBright · 2017-03-12T01:17:39Z

std/algorithm/iteration.d

            while (!r.empty)
            {
-                cast(void) unaryFun!pred(r.front);
+                static if (!is(typeof(unaryFun!fun(r.front)) == Flag!"each"))


swap the then and else clauses to eliminate the !

Nah, then I have a dangling else and need to add gnarly {}

The ! is a cognitive problem. Negations in general should be avoided. I missed it myself the first time through, and got mildly confused for a moment.

WalterBright · 2017-03-12T01:18:07Z

std/algorithm/iteration.d

            while (!r.empty)
            {
-                cast(void) binaryFun!BinaryArgs(i, r.front);
+                static if (!is(typeof(binaryFun!BinaryArgs(i, r.front)) ==


WalterBright · 2017-03-12T01:18:47Z

std/algorithm/iteration.d

-                cast(void) binaryFun!BinaryArgs(i, r.front);
+                static if (!is(typeof(binaryFun!BinaryArgs(i, r.front)) ==
+                        Flag!"each"))
+                    cast(void) binaryFun!BinaryArgs(i, r.front);


not sure what the cast(void) is good for

It was there. Chesterton's fence...

It's a common stylistic practice in other languages, in some it is required. The function shouldn't be returning a significant value anyway.

It's required if binaryFun is a strongly pure function and has a return type of void.

@MetaLang hmmm, yah but in that case it seems the cast precludes a potentially useful warning. This also ties in somehow with @schveiguy's discussion of free.

Depends on whether you want each to only accept non-strongly-pure functions or not. My first thought is that it's fine because why would you use each if you don't want to do something impure? However, if you want each to be as close in functionality to foreach as possible, then strongly-pure functions should be allowed as it is perfectly acceptable to have a "pure" foreach loop:

foreach (i; [0, 1, 2]) { //This loop has no side-effects }

That said, if I understand how strong-pure inference works, this code should not be accepted by the current DMD:

[0, 1, 2].each!((i) {}); //Should not compile

However, this code is accepted by DMD 2.037.2, so it looks like a non-issue (and now makes me wonder why this is accepted). If a problem ever does come up we can deal with it then.

WalterBright · 2017-03-12T01:21:34Z

std/algorithm/iteration.d

+        auto result = Yes.each;
+        auto dg(Parameters!(Parameters!(r.opApply)) params)
+        {
+            static if (!is(typeof(binaryFun!BinaryArgs(i, e)) == Flag!"each"))


is(typeof(binaryFun!BinaryArgs(i, e)) == Flag!"each") appears repeatedly. Factor it out and use it to initialize a manifest constant.

How? Zat iz ze queschiohn.

enum isEach = is(typeof(binaryFun!BinaryArgs(i, e)) == Flag!"each"); ... static if (isEach) ...

sehr gut, ja?

@WalterBright there is no improvement - essentially this introduces one additional name that is used only once. No?

WalterBright · 2017-03-12T01:22:46Z

std/algorithm/iteration.d

        (isForeachUnaryIterable!R || isForeachBinaryIterable!R);

-    void each(Range)(Range r)
+    Flag!"each" each(Range)(Range r)


Consider using static if instead of these overloads.

They don't appear in the docs so choosing one or another is a wash. I preserved the existing structure.

JackStouffer · 2017-03-12T03:41:13Z

What's the use case?

wilzbach · 2017-03-12T07:51:39Z

@andralex FYI: there are still multiple issues with each, but currently I lack the time and laptop to hunt the cycle issue down.
-> #4990

quickfur · 2018-01-26T19:55:17Z

ping @andralex @WalterBright

What's needed to get this moving again?

dlang-bot · 2018-08-14T13:51:32Z

Thanks for your pull request, @andralex!

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.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#5268"

wilzbach · 2018-08-14T13:53:01Z

What's needed to get this moving again?

Someone to revive it from the dead and rebase to latest, greatest master. I just did so ;-)

PetarKirov · 2018-08-15T08:54:36Z

@wilzbach needs another rebase

wilzbach · 2018-08-15T12:18:14Z

std/algorithm/iteration.d

        is(typeof((R r) {
            foreach (i, ref a; r)
-                cast(void) unaryFun!pred(i, a);
+                cast(void) binaryFun!BinaryArgs(i, a);


This was incorrect.

wilzbach · 2018-08-15T12:18:48Z

std/algorithm/iteration.d

+                    }
+                    else
+                    {
+                        if (unaryFun!fun(r.front) == No.each) return No.each;


The braces are required to avoid DScanner's "Mismatched static if. Use 'else static if' here." warning.

wilzbach · 2018-08-15T12:20:25Z

needs another rebase

Done. I also found a small issue with the newly added isForeachUnaryWithIndexIterable

andralex · 2018-08-15T22:58:33Z

@wilzbach did you adopt this PR? If so, how does that work? Thx!

wilzbach · 2018-08-15T23:02:46Z

@wilzbach did you adopt this PR?

Yes - it's all green now =)

If so, how does that work? Thx!

I use my pr_push wrapper ;-)

andralex · 2018-08-15T23:12:27Z

I guess it's legal to merge then!

wilzbach · 2018-08-15T23:20:27Z

Ah I still wanted to squash the commits, but I guess it's too late now :/

BTW I realized that it might be nice to highlight this change in the release changelog and submitted a changelog entry for it: #6668

andralex changed the title ~~Make each capable of early stopping~~ Make each capable of early stopping Mar 12, 2017

WalterBright reviewed Mar 12, 2017

View reviewed changes

JackStouffer added Merge:Needs Rebase Review:Needs Work labels Apr 6, 2017

dlang-bot added Merge:Needs Rebase and removed Merge:Needs Rebase labels Jun 22, 2017

dlang-bot added Merge:stalled Merge:Needs Rebase and removed Merge:Needs Rebase labels Dec 29, 2017

wilzbach force-pushed the each-with-early-stopping branch from ddb8863 to ae5d3ae Compare August 14, 2018 13:51

andralex requested review from JackStouffer, PetarKirov and wilzbach as code owners August 14, 2018 13:51

dlang-bot removed Merge:Needs Rebase Review:Needs Work labels Aug 14, 2018

dlang-bot removed the Merge:stalled label Aug 14, 2018

andralex and others added 4 commits August 15, 2018 11:06

Make each capable of early stopping

662acd4

It's Yes...

b803ffa

More doc fixes

8ef426d

Fix early stopping PR

e1d3720

wilzbach force-pushed the each-with-early-stopping branch 2 times, most recently from b1701ac to 005a391 Compare August 15, 2018 12:19

wilzbach reviewed Aug 15, 2018

View reviewed changes

wilzbach force-pushed the each-with-early-stopping branch from 005a391 to 6ef73fb Compare August 15, 2018 13:04

Another merge

6ef73fb

wilzbach approved these changes Aug 15, 2018

View reviewed changes

andralex merged commit a12d990 into dlang:master Aug 15, 2018

wilzbach mentioned this pull request Aug 15, 2018

Add a changelog entry for each's early stopping #6668

Merged

Uh oh!

Comments

Conversation

andralex commented Mar 12, 2017

Uh oh!

andralex commented Mar 12, 2017

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!

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!

MetaLang Mar 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MetaLang Mar 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

JackStouffer commented Mar 12, 2017

Uh oh!

wilzbach commented Mar 12, 2017

Uh oh!

quickfur commented Jan 26, 2018

Uh oh!

dlang-bot commented Aug 14, 2018

Bugzilla references

Testing this PR locally

Uh oh!

wilzbach commented Aug 14, 2018

Uh oh!

PetarKirov commented Aug 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Aug 15, 2018

Uh oh!

andralex commented Aug 15, 2018

Uh oh!

wilzbach commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andralex commented Aug 15, 2018

Uh oh!

MetaLang Mar 12, 2017 •

edited

Loading

MetaLang Mar 12, 2017 •

edited

Loading

wilzbach commented Aug 15, 2018 •

edited

Loading