fix Issue 12409 - Add "each" function as found in Ruby and jQuery #2024

CyberShadow · 2014-03-19T06:32:30Z

https://d.puremagic.com/issues/show_bug.cgi?id=12409

andralex · 2014-03-19T06:46:32Z

std/algorithm.d

+unittest
+{
+    int[] arr;
+    iota(5).each!(n => arr ~= n);


vs

foreach (n; iota(5)) { arr ~= n; }

About equivalent, though it's nice that the range comes first if it's a long pipeline.

Yep, that's the point :) See the example in the DDoc and the discussion in the Bugzilla issue.

About equivalent, though it's nice that the range comes first if it's a long pipeline.

While fine as a test, this shouldn't be taken as an example of anything. The better way to do this would be:

auto arr = appender!(int[])(); iota(5).copy(arr);

JakobOvrum · 2014-03-19T07:41:01Z

I've always been against this. We have a fairly nice foreach statement that does exactly the same thing but in ~~a declarative~~ an imperative and not functional style, as is proper, as this is ~~declarative~~ imperative code no matter how you disguise it.

We also shouldn't take after Ruby on this; Ruby inherits the Perl way of providing a ton of ways to express the same thing, which I think is anti-thetical to the D approach.

edit:

Sorry, s/declarative/imperative. The functional code is declarative but ceases to be so when imperative code is injected into the chain.

CyberShadow · 2014-03-19T08:52:15Z

We have a fairly nice foreach statement that does exactly the same thing but in a declarative and not functional style, as is proper, as this is declarative code no matter how you disguise it.

That's a rather fundamental look at the issue. But I don't think it holds up. We already have functions which eagerly evaluate its first range parameters, such as copy.

We also shouldn't take after Ruby on this; Ruby inherits the Perl way of providing a ton of ways to express the same thing, which I think is anti-thetical to the D approach.

I don't see why you think so. D is a multi-paradigm language, so by definition it provides multiple ways to accomplish the same thing - in different paradigms.

The same goes for UFCS. It would not have existed with that logic: it provides two ways to do the same thing, and it allows mixing programming styles. Yet it thrives as one of D's most important features.

monarchdodra · 2014-03-19T09:13:37Z

I have less reservations about this than for "tee" in regards to the whole imperative code in functional style: "each" is a tail call, and the iteration scheme is clearly defined. It does not produce a range.

That said, I'm starting to wonder if there might not be a more "generic" way of achieving this: "tee" is still under review, and if it passes, then "each" could be replaced by a tee(fun).walk() (provided walk also passes).

So my stance on each is that while I don't reject it, I consider it is currently waiting its spot in the review queue.

andralex · 2014-03-19T15:08:00Z

each is better than walk and in particular can be defaulted such that each() does what walk does.

andralex · 2014-03-19T15:12:13Z

I think a good analysis of each vs foreach would help here. For example: each cannot be easily broken out of, so if that's the intent it expresses it closely. Also the whole expression vs statement aspect may be important, for example simple lambdas x => expr can be written with each but not with foreach.

There must be others. Let's collect a list.

Oh, and also: @CyberShadow you should define each with two parameters as well.

monarchdodra · 2014-03-19T15:13:15Z

each is better than walk

Not sure why "better". They are different.

and in particular can be defaulted such that each() does what walk does.

That's a good point. Would each() still make calls to front though, or just pop everything?

andralex · 2014-03-19T15:13:30Z

One more thought: if walk has any viable raison d'être then that makes the introduction of each a foregone conclusion.

andralex · 2014-03-19T15:13:52Z

(github ain't that good at Unicode eh)

andralex · 2014-03-19T15:14:39Z

@monarchdodra depends on the default lambda of each.

monarchdodra · 2014-03-19T15:15:46Z

raison d'Ãªtre

(github ain't that good at Unicode eh)

Must be the code page your computer is using, and not github/unicode. "Raison d'être". 日本語.

monarchdodra · 2014-03-19T15:17:03Z

@monarchdodra depends on the default lambda of each.

If each merelly has the default "a" lambda, then it will call front. walk wouldn't. Not saying one is better or anything, just pointing it out.

ntrel · 2014-03-19T16:09:02Z

std/algorithm.d

+    auto cutoff = Clock.currTime() - 7.days;
+    dirEntries("", "*~", SpanMode.depth)
+        .filter!(de => de.timeLastModified < cutoff)
+        .each!remove();


It's nice you can call remove in this point-free style, without needing a variable.

mleise · 2014-03-19T16:14:27Z

If the function passed to each returned one of

enum ControlFlow { _break, _continue }

you could break out of each(). If it has a different return type nothing happens. If you really want that functionality in each, but I think it is over-engineering.

yebblies · 2014-03-19T16:21:09Z

Or just use filter and until. Or full foreach.

andralex · 2014-03-19T16:39:15Z

Yah I think not being able to break is a feature, not an insufficiency of each.

JakobOvrum · 2014-03-19T20:28:56Z

That's a rather fundamental look at the issue. But I don't think it holds up. We already have functions which eagerly evaluate its first range parameters, such as copy.

Algorithms that eagerly consume a range are perfectly fine, and copy is a fine example of one. The problem is with an eager algorithm that exists entirely to insert ad-hoc eagerness into a functional chain, relying entirely or partly on side-effects of the chain. That's simply not functional, that's imperative.

I don't see why you think so. D is a multi-paradigm language, so by definition it provides multiple ways to accomplish the same thing - in different paradigms.

The problem is with interfaces that don't differ in anything but style, i.e. aliases. Ruby's Array type is a good example of how Ruby has no qualms packing tons of trivia helpers and aliases (count/size/length anyone?) onto an interface. Infix if and unless is another example.

andralex · 2014-03-19T21:13:33Z

I think we could and should judge this on its own merit rather than drawing Ruby comparisons etc.

WalterBright · 2014-03-19T22:57:29Z

std/algorithm.d

+    {
+        unaryFun!fun(range.front);
+        range.popFront();
+    }


Seems to me this loop could be implemented as:

foreach (e; range) unaryFun!(e);

Existing form is a tad better because it doesn't create a copy in certain cases.

foreach (ref e; range) unaryFun!(e);

Huh, that works. ref is auto ref when it is in a foreach?

It won't work properly with ranges of which front returns rvalues. Just let it be.

That won't work for narrow strings.

@yebblies: noice

These two issues may as well get each over the hump!

That won't work for narrow strings.

foreach (ref ElementType!Range e; range) unaryFun!(e);

This is the approach used in count:
https://github.com/D-Programming-Language/phobos/blob/9404dd477fd59925acd958cb0cf21711890420d7/std/algorithm.d#L6663

The fact that ref in foreach binds to rvalues is a bug in the design of D.

All code that does that is wrong. We should break it and replace it with an auto ref feature instead.

Existing form is a tad better because it doesn't create a copy in certain cases.

It also doesn't create a copy of range when it's a ForwardRange, though it seems here that might actually be desired (not currently done).

Hackerpilot · 2014-03-19T23:18:07Z

I'm in favor of adding this.

andralex · 2014-03-20T15:50:07Z

I'm getting convinced this is useful/nontrivial. There are a few things that should be added:

Unittests for lambdas taking by ref (should work with ranges that have lvalue front but not rvalue front)
Two-arguments version that passes 0, 1, 2... as the first argument. First argument cannot be ref (maybe later).
I wonder what to do about each with no arguments. Should it exist, and if so should if call front or not? (maybe later)

monarchdodra · 2014-03-20T15:56:52Z

I wonder what to do about each with no arguments.

For what it's worth, just make pred default to "a", and you're golden. No special casing the code, and consistent behavior. So might as well "simply just do that"

Note that the "consistent behavior" is important (iMO), or you'd get a different behavior from each() and each!"a"(), which I don't think would make sense.

Two-arguments version that passes 0, 1, 2... as the first argument.

Not sure what you are asking for. Could you elaborate?

andralex · 2014-03-20T18:11:02Z

I wonder what to do about each with no arguments.
For what it's worth, just make pred default to "a", and you're golden. No special casing the code, and consistent behavior. So might as well "simply just do that"

Note that the "consistent behavior" is important (iMO), or you'd get a different behavior from each() and each!"a"(), which I don't think would make sense.

I guess.

Two-arguments version that passes 0, 1, 2... as the first argument.
Not sure what you are asking for. Could you elaborate?

Enumerate, like foreach does.

iota(1, 3).each!((i, n) => writeln(i, ":", n, " ")); // 0:1 1:2

YotaXP · 2014-03-20T18:47:46Z

I was actually looking for this function in Phobos just yesterday, and was surprised to not find it.
Nice timing! 👍

As a side thought, I imagine that the call to front could be made optional by passing a delegate that has a lazy parameter. That could give this a potential edge over foreach.

JakobOvrum · 2014-03-21T03:37:18Z

Enumerate, like foreach does.

foreach only does that when the aggregate is an array. There is #1866 to make this behaviour composable for any range, though.

(Note I'm still fundamentally opposed to this function, especially if the template parameter is optional.)

CyberShadow · 2014-03-23T17:05:34Z

Sorry to keep everyone waiting. Catching up now.

TODO

Design:

Use foreach (ref ElementType!Range e; range) ?
- No: foreach iteration cannot satisfy the requirement that ref lambdas fail to compile if front does not return by ref. (But maybe we could check for this explicitly, by passing an rvalue in the template constraint?)
Decide on what to do about each with no arguments? Have "a" as the default predicate?

Implementation:

Unittests for lambdas taking by ref (should work with ranges that have lvalue front but not rvalue front)
Two-arguments version that passes 0, 1, 2... as the first argument.
..... First argument cannot be ref
(maybe later).
Add overload for opApply objects, which would allow interoperability with e.g. std.parallelism.parallel.
Add overload for arrays (which will use foreach), which should result in fast code for e.g. arr.each!"a++"

CyberShadow · 2014-03-23T19:56:30Z

For what it's worth, just make pred default to "a", and you're golden. No special casing the code, and consistent behavior. So might as well "simply just do that"

I think each with no arguments is a rather unintuitive use for it from a code readability point. Can we not have a default argument for each and instead define alias walk = each!"a"?

CyberShadow · 2014-03-23T20:14:53Z

First argument cannot be ref (maybe later).

binaryFun cannot enforce that restriction (it always uses auto ref). We'd need to either expand binaryFun or use our own thing.

Hackerpilot · 2014-12-04T21:23:24Z

I'm still in favor of adding this. Who makes the decision on merging or rejecting?

quickfur · 2014-12-04T22:01:29Z

Dunno... generally we committers merge when there is consensus, but @Dicebot seems to be in disagreement on this PR and there hasn't been any arguments for merging since then. So I'm holding off on deciding on anything just yet. You might want to ping the other reviewers/committers.

MetaLang · 2014-12-05T00:10:27Z

One thing to note is that a std.algorithm.sum function was added with the argument that the functionality it had in common with reduce was justified as sum could be made faster in the special case of summation. Perhaps a similar argument could be made as each is faster for arrays according to @CyberShadow. Really, the ability to interoperate with std.parallelism.parallel alone justifies each.

WalterBright · 2014-12-31T07:14:01Z

std/algorithm.d

+with e.g. $(XREF parallelism, parallel).
+
+See_Also: $(XREF range,tee)
+


Missing Params: and Returns:

The description would make them redundant, no?

If the description is redundant, remove the description not the blocks.

I don't see a way to word it in that way without also making it unnecessarily awkward.

The function returns void. Why do you want me to add a Returns: block for a void function?

Sorry, didn't realize it returned void. I agree there should not be a Returns: block for void returns.

The Params: block should be:

Params: fun = predicate to apply to each element of the range range = range over which each iterates

Note that the original description describes a $(D r) parameter but there is no parameter r. The reason for a Params: section is because inconsistencies like this regularly and repeatedly recur.

And what if the parameter name varies depending on which overload you call? Should the parameter names be homogenized for the sake of Params:?

I think homogenizing them should be the best approach.

Agreed. The more structure the better we can organize documents and the more readable they'll be to users.

ghost · 2014-12-31T15:26:04Z

Anyone know why there's now 2 results for the autotester?

Failed — 2 failing checks 
default — Fail: 1, In Progress: 1, Pending: 8
auto-tester — Fail: 2, Pending: 8

WalterBright · 2014-12-31T22:52:00Z

std/algorithm.d

+        }
+    }
+
+    void each(Iterator)(Iterator iterator)


This is calling something an iterator when it is actually a range. Please call it a range. I'm also baffled by the idea introduced here that something can be a range and not be iterable? Why is there a whole new range (!) of terms being introduced?

No. In this case, it is actually an iterator. This overload handles objects which define opApply, not ranges.

Please call it something else. It has nothing in common with C++ iterators. The documentation on opApply never calls them iterators, it calls them apply functions.

http://dlang.org/statement.html#foreach-with-ranges

I wasn't thinking of C++ iterators when I chose the term, but more of those found in Python/Ruby/C#. What term would you suggest, then? (If not for the parameter name, then for the inferred type name).

How about apply function?

That's a subset of accepted parameters. It wouldn't be the right term for classes/structs which declare opApply, right?

Call it Iterable? That's the term used in phobos.

andralex · 2014-12-31T23:39:11Z

Regarding documentation, there is great advantage to using 'Params:', 'Returns:', 'Throws:' etc uniformly because they can be styled and handled in various ways when generating documentation. More structure can help a lot. (I do agree a function without 'Returns:' can be assumed to return void.)

CyberShadow · 2015-01-02T04:21:37Z

Implemented suggested changes and rebased.

ghost · 2015-01-02T10:20:47Z

LGTM. Always wanted this function. @andralex ready to merge?

andralex · 2015-01-10T17:20:17Z

std/algorithm.d

+        {
+            while (!r.empty)
+            {
+                cast(void)unaryFun!pred(r.front);


why the cast?

To silence the warning that we are throwing away the return value. (x4)

andralex · 2015-01-10T17:21:35Z

I think this code sans the casts is good to go. Thanks!

ghost · 2015-01-11T13:24:37Z

Auto-merge toggled on

ghost · 2015-01-11T13:24:50Z

Time to get this rolling methinks!

ghost · 2015-01-11T13:25:16Z

(I'm really excited for the next release, we've gotten so many new features recently in Phobos)

fix Issue 12409 - Add "each" function as found in Ruby and jQuery

nordlow · 2015-01-11T14:29:35Z

Somewhat related I now wonder about the soundness of map in

import std.algorithm, std.range, std.stdio;
void main(string[] args)
{
    long[] arr;
    const n = 3;
    iota(n).map!(a => arr ~= a);
    writeln(arr);
    writeln(iota(n).map!(a => arr ~= a));
    writeln(arr);
}

that prints

[]
[[0], [0, 1], [0, 1, 2]]
[0, 1, 2]

Shouldn't a warning at least be issued for return-ignoring calls to map with mutating lambdas? Has there been any discussions on making map require pure functions now that we have each? I guess a new function, say mapPure, may be neccessary as adding such a restriction to the lambda will break too much code right?

BTW: Should I turn this into a discussion on the forums instead?

bearophile · 2015-01-11T16:34:05Z

I have reopened the issue with some comments: https://issues.dlang.org/show_bug.cgi?id=12409

andralex · 2015-01-11T17:13:41Z

Nice work!

andralex · 2015-01-11T17:14:15Z

@nordlow forum would probably be best

andralex reviewed Mar 19, 2014
View reviewed changes

ntrel reviewed Mar 19, 2014
View reviewed changes

WalterBright reviewed Mar 19, 2014
View reviewed changes

CyberShadow changed the title ~~fix Issue 12409 - Add "each" function as found in Ruby and jQuery~~ [WIP] fix Issue 12409 - Add "each" function as found in Ruby and jQuery Mar 23, 2014

CyberShadow force-pushed the fix12409 branch from a61585f to c5d8492 Compare October 5, 2014 16:47

9il mentioned this pull request Nov 8, 2014

std.range package #2661

Merged

WalterBright reviewed Dec 31, 2014
View reviewed changes

CyberShadow force-pushed the fix12409 branch from c5d8492 to ad411e4 Compare January 2, 2015 04:20

fix Issue 12409 - Add "each" function as found in Ruby and jQuery

bf14b8b

CyberShadow force-pushed the fix12409 branch from ad411e4 to bf14b8b Compare January 2, 2015 10:28

andralex reviewed Jan 10, 2015
View reviewed changes

ghost pushed a commit that referenced this pull request Jan 11, 2015

Merge pull request #2024 from CyberShadow/fix12409

f3604ad

fix Issue 12409 - Add "each" function as found in Ruby and jQuery

ghost merged commit f3604ad into dlang:master Jan 11, 2015

This pull request was closed.

		with e.g. $(XREF parallelism, parallel).

		See_Also: $(XREF range,tee)

Uh oh!

fix Issue 12409 - Add "each" function as found in Ruby and jQuery #2024

fix Issue 12409 - Add "each" function as found in Ruby and jQuery #2024

Uh oh!

Conversation

CyberShadow commented Mar 19, 2014

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!

JakobOvrum commented Mar 19, 2014

Uh oh!

CyberShadow commented Mar 19, 2014

Uh oh!

monarchdodra commented Mar 19, 2014

Uh oh!

andralex commented Mar 19, 2014

Uh oh!

andralex commented Mar 19, 2014

Uh oh!

monarchdodra commented Mar 19, 2014

Uh oh!

andralex commented Mar 19, 2014

Uh oh!

andralex commented Mar 19, 2014

Uh oh!

andralex commented Mar 19, 2014

Uh oh!

monarchdodra commented Mar 19, 2014

Uh oh!

monarchdodra commented Mar 19, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mleise commented Mar 19, 2014

Uh oh!

yebblies commented Mar 19, 2014

Uh oh!

andralex commented Mar 19, 2014

Uh oh!

JakobOvrum commented Mar 19, 2014

Uh oh!

andralex commented Mar 19, 2014

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!

Hackerpilot commented Mar 19, 2014

Uh oh!

andralex commented Mar 20, 2014

Uh oh!

monarchdodra commented Mar 20, 2014

Uh oh!

andralex commented Mar 20, 2014

Uh oh!

YotaXP commented Mar 20, 2014

Uh oh!

JakobOvrum commented Mar 21, 2014

Uh oh!

CyberShadow commented Mar 23, 2014