Refactor std.algorithm.iteration #7638

andralex · 2020-09-18T00:28:55Z

The refactorings involve simplifying code that was working around compiler bugs, pushing symbols from top level to where they are needed, and a few more simplifications.

dlang-bot · 2020-09-18T00:28:57Z

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 run digger -- build "master + phobos#7638"

andralex · 2020-09-18T00:29:48Z

std/algorithm/iteration.d

+        alias _funs = AliasSeq!(_fun);
+    }
+
+    struct Result(Range)


moved from top level

andralex · 2020-09-18T00:30:48Z

std/algorithm/iteration.d

@@ -955,11 +920,19 @@ private:
         isForeachUnaryWithIndexIterable!R);

 public:
+    Flag!"each" each(Range)(auto ref Range r)
+    if (isForeachIterable!Range ||


Largely unsuccessful attempt to make the constraint nicer. Will get back to this.

andralex · 2020-09-18T00:31:11Z

std/algorithm/iteration.d

+        return Result!Range(range);
+    }
+
+    private struct Result(Range)


moved from top level

andralex · 2020-09-18T00:31:26Z

std/algorithm/iteration.d

+        return Result!Range(r);
+    }
+
+    private struct Result(Range)


moved from top level

andralex · 2020-09-18T00:32:07Z

std/algorithm/iteration.d

 // Implementation of chunkBy for non-forward input ranges.
-private struct ChunkByImpl(alias pred, Range)
+private struct ChunkByImpl(alias pred, alias eq, bool isUnary, Range)


Could move this inside chunkBy as well, but it's fairly bulky.

andralex · 2020-09-18T00:32:42Z

std/algorithm/iteration.d

@@ -1973,48 +1894,69 @@ if (isInputRange!Range && !isForwardRange!Range)
    @property auto front()
    {
        assert(!empty, "Attempting to fetch the front of an empty chunkBy.");
+
+        static struct Result


this was small enough to plop here

andralex · 2020-09-18T00:33:03Z

std/algorithm/iteration.d

        }
    }

    void popFront()
    {
        assert(!empty, "Attempting to popFront an empty chunkBy.");
        openChunk = false;
-        while (!r.empty)
+        for (; !r.empty; r.popFront)


my dream: that more loops are written like this

andralex · 2020-09-18T00:33:23Z

std/algorithm/iteration.d

        }
    }
 }

 // Single-pass implementation of chunkBy for forward ranges.
-private struct ChunkByImpl(alias pred, Range)
+private struct ChunkByImpl(alias pred, alias eq, bool isUnary, Range)


this is also too bulky to move inside chunkBy

andralex · 2020-09-18T00:34:07Z

std/algorithm/iteration.d

+    static if (isUnary)
+        alias eq = binaryFun!((a, b) => unaryFun!pred(a) == unaryFun!pred(b));
+    else
+        alias eq = binaryFun!pred;


this code was duplicated across the two implementations of ChunkByImpl.

andralex · 2020-09-19T04:03:18Z

std/algorithm/iteration.d

-        isRangeIterable!Range ||
-        __traits(compiles, typeof(r.front).length)))
+    public Flag!"each" each(Range)(auto ref Range range)
+    if (isInputRange!Range || isStaticArray!Range || hasMember!(Range, "opApply"))


This is a simpler condition (albeit incomplete because it doesn't verify the function). Also the entire implementation is a lot shorter.

Checked the speed and memory consumed - this version is about as fast and eats 0.1% less memory.

andralex · 2020-09-19T04:15:30Z

std/algorithm/iteration.d


 Returns: `Yes.each` if the entire range was iterated, `No.each` in case of early
 stopping.

 See_Also: $(REF tee, std,range)
 */
-template each(alias fun = "a")
+public Flag!"each" each(alias fun = "a", Range)(auto ref Range range)
+if (isInputRange!Range || isStaticArray!Range || hasMember!(Range, "opApply"))


whew, nice, it can be reduced to a single function

I don't think this is something that should be celebrated.

static if (is(typeof(f1!()))) return f1(); else static if (is(typeof(f2!()))) return f2(); else static if (is(typeof(f3!()))) return f3(); else static if (is(typeof(f4!()))) return f4(); else static if (is(typeof(f5!()))) return f5(); else static if (is(typeof(f6!()))) return f6(); else static if (is(typeof(f7!()))) return f7(); else static assert(0, "Cannot iterate " ~ Range.stringof ~ " with each().");

The "celebration" was in regard to moving each from a member in a template to a top-level function.

I agree that the "try each pattern and see what works" strategy does raise an eyebrow, and am always on lookout for better solutions - please advise. I took inspiration from @pbackus's amazing sumtype library which works almost miraculously well at scale. There, you get to pass one or more template lambdas to a sum type and the library simply iterates all possibilities and picks the first that compiles. Experience with sumtype suggests that's a really solid idiom for D.

There are certain advantages to this strategy as implemented here compared to the existing code:

It's easy to understand and maintain - each pattern is in the clear. This is a very important aspect! It's very difficult to make heads and tails of the code otherwise. Once you see what patterns the library is looking for, it's very easy to get into it.

It's much more terse than the baseline - this PR deletes 166 lines, many of which I think are attributable to this change.

There's good precedent with sumtype

dukc · 2021-02-22T20:39:46Z

Good work - just a little nicer to read all over the module. Modulo getting the test suite to pass, LGTM.

andralex · 2021-02-22T22:06:17Z

whoa blast from the past

RazvanN7 · 2021-04-15T09:52:20Z

@andralex what are your plans with regards to this PR?

andralex requested review from JackStouffer and wilzbach as code owners September 18, 2020 00:28

andralex commented Sep 18, 2020

View reviewed changes

std/algorithm/iteration.d

alias _funs = AliasSeq!(_fun);

}

struct Result(Range)

Copy link

Member Author

andralex Sep 18, 2020

Choose a reason for hiding this comment

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

moved from top level

andralex commented Sep 18, 2020

View reviewed changes

std/algorithm/iteration.d

return Result!Range(range);

}

private struct Result(Range)

Copy link

Member Author

andralex Sep 18, 2020

Choose a reason for hiding this comment

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

moved from top level

andralex commented Sep 18, 2020

View reviewed changes

std/algorithm/iteration.d

return Result!Range(r);

}

private struct Result(Range)

Copy link

Member Author

andralex Sep 18, 2020

Choose a reason for hiding this comment

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

moved from top level

andralex commented Sep 18, 2020

View reviewed changes

andralex force-pushed the iteration1 branch from 6968eff to 82e8a0c Compare September 19, 2020 03:50

andralex commented Sep 19, 2020

View reviewed changes

andralex force-pushed the iteration1 branch from 82e8a0c to de48263 Compare September 19, 2020 04:14

andralex commented Sep 19, 2020

View reviewed changes

andralex force-pushed the iteration1 branch from de48263 to 27e293c Compare September 19, 2020 04:15

andralex changed the title ~~Refactor std.algorithm.iteration through chunkBy~~ Refactor std.algorithm.iteration Sep 19, 2020

andralex force-pushed the iteration1 branch 6 times, most recently from 1392a1c to 964a932 Compare September 20, 2020 15:53

jondegenhardt mentioned this pull request Sep 20, 2020

tsv-summarize: Slice SummarizerBase._operators when invoking std.algorithm.each eBay/tsv-utils#308

Open

andralex added 5 commits September 22, 2020 22:06

Refactor std.algorithm.iteration through chunkBy

f3738e7

Redo each

c826c1a

Redo each constraints and implementation

4f7c40f

A little refactoring of the reduce nightmare

b51d04d

Factor opSlice forwarding implementation into a common template

b5eaa6b

Support types that are not ranges but define a range-returning opSlice

71bbed7

andralex force-pushed the iteration1 branch from 964a932 to 71bbed7 Compare September 23, 2020 02:10

dlang-bot added Needs Rebase Needs Work labels May 27, 2021

dlang-bot added the stalled label Jul 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor std.algorithm.iteration #7638

Refactor std.algorithm.iteration #7638

andralex commented Sep 18, 2020

dlang-bot commented Sep 18, 2020

andralex Sep 18, 2020

andralex Sep 18, 2020

andralex Sep 18, 2020

andralex Sep 18, 2020

andralex Sep 18, 2020

andralex Sep 18, 2020

andralex Sep 18, 2020

andralex Sep 18, 2020

andralex Sep 18, 2020

andralex Sep 19, 2020

andralex Sep 19, 2020

looked-at-me Sep 20, 2020

andralex Sep 20, 2020

dukc commented Feb 22, 2021

andralex commented Feb 22, 2021

RazvanN7 commented Apr 15, 2021

Refactor std.algorithm.iteration #7638

Are you sure you want to change the base?

Refactor std.algorithm.iteration #7638

Conversation

andralex commented Sep 18, 2020

dlang-bot commented Sep 18, 2020

Bugzilla references

Testing this PR locally

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dukc commented Feb 22, 2021

andralex commented Feb 22, 2021

RazvanN7 commented Apr 15, 2021