[RFC] Make std.algorithm.searching.skipOver an eponymous template to allow partial instantiation by wilzbach · Pull Request #5576 · dlang/phobos

wilzbach · 2017-07-08T00:40:04Z

Argh! find does not use the eponymous template pattern, which means it's not partially instantiatable with just a pred.

It turns out that many functions in std.algorithm don't, so this is a RFC for evaluating the best way to move forward here. I picked the non-trivial function skipOver as an example.

dlang-bot · 2017-07-08T00:40:05Z

Thanks for your pull request, @wilzbach!

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.

wilzbach · 2017-07-08T00:42:01Z

std/algorithm/searching.d

    pred = The predicate that determines whether elements from each respective
        range match. Defaults to equality $(D "a == b").
+*/
+template skipOver(alias pred = "a == b")


This is the eponymous pattern also used by other methods in this module (e.g. canFind, all or any).

wilzbach · 2017-07-08T00:45:17Z

std/algorithm/searching.d

+        isInputRange!R2)
    {
-        if (r2.length > r1.length || r1[0 .. r2.length] != r2)
+        static if (is(typeof(pred) : string) && pred == "a == b"


I am aware that @andralex isn't a huge fan of this pattern, but without this, we can't have a default pred for skipOver and thus would need to duplicate all overloads (as previously done).

If alternatives are a lot of aggravationm, then fine. But seeing template skipOver is already a sign that something has gotten too complicated...

But seeing template skipOver is already a sign that something has gotten too complicated...

Making skipOver is fully on purpose and what this entire informal RFC PR is about.

MetaLang · 2017-07-09T19:48:01Z

I hate this pattern too. Can't we just add another template "overload" that only takes a predicate and then forwards to another version of skipOver?

wilzbach · 2017-07-10T03:01:35Z

I hate this pattern too. Can't we just add another template "overload" that only takes a predicate and then forwards to another version of skipOver?

How would this work without duplicating all function headers of skipOver?

MetaLang · 2017-07-10T15:46:04Z

Yeah, true. Looks like it can't be helped.

CyberShadow · 2017-07-10T22:17:50Z

But seeing template skipOver is already a sign that something has gotten too complicated...

Err, isn't this the by-the-book standard eponymous template pattern which allows partial instantiation and is already used in lots of places in Phobos, such as map?

wilzbach · 2017-07-10T23:25:43Z

Btw another benefit of partial instantiation is that we get Dependency-Carrying Declarations a la DIP1005 for free :)
I wonder why DIP1005 didn't talk about this.
Consider a simple example:

template foo(alias pred = "a == b")
{
    import std.range.primitives : isInputRange;
    auto foo(R)(R r)
    if (isInputRange!R)
    {
        import std.algorithm.iteration : sum;
        return r.sum;
    }
}

void main()
{
    foo([1]); // <- remove this and no modules are imported
}

> rdmd -v foo.d
predefs   DigitalMars Posix linux ELFv1 LittleEndian D_Version2 all D_SIMD D_InlineAsm_X86_64 X86_64 CRuntime_Glibc D_LP64 D_PIC assert D_HardFloat
binary    /usr/bin/dmd
version   v2.074.0
config    /etc/dmd.conf
parse     foo
importall foo
import    object	(/usr/include/dlang/dmd/object.d)
semantic  foo
entry     main      	foo.d
semantic2 foo
semantic3 foo
import    std.range.primitives	(/usr/include/dlang/dmd/std/range/primitives.d)
import    std.traits	(/usr/include/dlang/dmd/std/traits.d)
import    std.typetuple	(/usr/include/dlang/dmd/std/typetuple.d)
import    std.meta	(/usr/include/dlang/dmd/std/meta.d)
import    std.algorithm.iteration	(/usr/include/dlang/dmd/std/algorithm/iteration.d)
import    std.functional	(/usr/include/dlang/dmd/std/functional.d)
import    std.ascii	(/usr/include/dlang/dmd/std/ascii.d)
import    std.algorithm.internal	(/usr/include/dlang/dmd/std/algorithm/internal.d)
code      foo
function  D main
function  std.range.primitives.front!int.front
function  foo.foo!().foo!(int[]).foo
function  std.algorithm.iteration.sum!(int[]).sum
function  std.algorithm.iteration.sum!(int[], int).sum
function  std.algorithm.iteration.reduce!"a + b".reduce!(int, int[]).reduce
function  std.algorithm.iteration.reduce!"a + b".reducePreImpl!(int[], int).reducePreImpl
function  std.algorithm.iteration.reduce!"a + b".reduceImpl!(false, int[], int).reduceImpl
function  std.functional.F!("a + b", "a", "b").binaryFun!(int, int).binaryFun
cc /tmp/.rdmd-1000/rdmd-foo.d-14302BAA00C11879E03D4743294EEEA6/objs/foo.o -o /tmp/.rdmd-1000/rdmd-foo.d-14302BAA00C11879E03D4743294EEEA6/foo.tmp -m64 -L/usr/lib -Xlinker --export-dynamic -Xlinker -Bstatic -lphobos2 -Xlinker -Bdynamic -lpthread -lm -lrt -ldl

Now without the use of the eponymous template:

> rdmd -v foo.d
predefs   DigitalMars Posix linux ELFv1 LittleEndian D_Version2 all D_SIMD D_InlineAsm_X86_64 X86_64 CRuntime_Glibc D_LP64 D_PIC assert D_HardFloat
binary    /usr/bin/dmd
version   v2.074.0
config    /etc/dmd.conf
parse     foo
importall foo
import    object	(/usr/include/dlang/dmd/object.d)
semantic  foo
entry     main      	foo.d
semantic2 foo
semantic3 foo
code      foo
function  D main
cc /tmp/.rdmd-1000/rdmd-foo.d-14302BAA00C11879E03D4743294EEEA6/objs/foo.o -o /tmp/.rdmd-1000/rdmd-foo.d-14302BAA00C11879E03D4743294EEEA6/foo.tmp -m64 -L/usr/lib -Xlinker --export-dynamic -Xlinker -Bstatic -lphobos2 -Xlinker -Bdynamic -lpthread -lm -lrt -ldl

wilzbach · 2017-07-17T17:33:10Z

@andralex so what's your opinion on this? Can we move forward?

andralex · 2017-07-25T16:29:31Z

So the current crop of skipOver overloads is:

bool skipOver(R1, R2)(ref R1 r1, R2 r2)
if (isForwardRange!R1
	&& isInputRange!R2
    && is(typeof(r1.front == r2.front)));

bool skipOver(alias pred, R1, R2)(ref R1 r1, R2 r2)
if (isForwardRange!R1
	&& isInputRange!R2
    && is(typeof(binaryFun!pred(r1.front, r2.front))));

bool skipOver(alias pred, R)(ref R r1)
if (isForwardRange!R
	&& ifTestable!(typeof(r1.front), unaryFun!pred));

bool skipOver(R, E)(ref R r, E e)
if (isInputRange!R && is(typeof(r.front == e) : bool));

bool skipOver(alias pred, R, E)(ref R r, E e)		
if (is(typeof(binaryFun!pred(r.front, e))) && isInputRange!R);

My first notes independent on this PR:

There are a few mistakes - the type of == and predicate must be if-testable. Only one overload gets that right!
It seems the constraint on the third overload is too tight - the range can be input, not forward
There are a few forms of duplication in the constraints:
- equality or predicates should always work the same
- in the one-range version the range must be input
- in the two-range version the first must be forward and the second must be input

Based on the above I'd say this PR may be worth it even if we discount for the advantages of partial instantiation and import.

I'll now make a pass through the code looking for details.

andralex

OK I'm fine with this. Before we open the floodgates to reworking everything this way, if this pattern is too frequent we may be better off with a small language addition (something like allowing overloading on template alias comes to mind).

andralex · 2017-07-25T16:33:15Z

std/algorithm/searching.d

+    $(D r1) is left in its original position.
+    */
+    bool skipOver(R1, R2)(ref R1 r1, R2 r2)
+    if (is(typeof(binaryFun!pred(r1.front, r2.front))) &&


This should be ifTestable (not related to this PR but can go with it because code disobeying that doesn't compile anyway)

andralex · 2017-07-25T16:35:14Z

std/algorithm/searching.d

+
+    /// Ditto
+    bool skipOver(R, E)(ref R r, E e)
+    if (is(typeof(binaryFun!pred(r.front, e))) && isInputRange!R)


predicate should be ifTestable

andralex · 2017-08-11T20:24:34Z

ping @wilzbach this is approved with a couple of nits, feel free to amend or commit as is

wilzbach · 2017-08-11T20:26:37Z

ping @wilzbach this is approved with a couple of nits, feel free to amend or commit as is

Thanks for the ping and I am well aware of it, but I have been pretty busy with life for the last days :/
Though I hopefully will get back to this soon :)

wilzbach · 2017-11-17T16:33:15Z

this is approved with a couple of nits, feel free to amend or commit as is

Okay I put this on the auto-merge queue now. IfTestable is only used in Phobos 10 times, so imho it's better to do these changes in a separate PR.

WalterBright · 2017-11-23T09:44:11Z

I turned off automerge because it was turned on for 6 days and did not merge due to failure(s) in CyberShadow/DAutoTest

WalterBright · 2017-11-23T09:46:31Z

ping @wilzbach

wilzbach · 2017-12-02T13:31:17Z

Merging manually as there seems to be something wrong with the CI status report from the auto-tester (probably it has sent more than 1000 status updates):

It's 100% green:

wilzbach added 2 commits July 8, 2017 02:36

Make std.algorithm.searching.skipOver an eponymous template

4a2a2da

Add unittest for partial instantiation

7fb31ac

wilzbach added the Review:Needs Decision label Jul 8, 2017

wilzbach commented Jul 8, 2017

View reviewed changes

wilzbach added the @andralex label Jul 17, 2017

wilzbach requested a review from andralex July 17, 2017 17:31

andralex approved these changes Jul 25, 2017

View reviewed changes

andralex removed @andralex Review:Needs Decision labels Jul 25, 2017

wilzbach added the Merge:auto-merge label Nov 17, 2017

WalterBright removed the Merge:auto-merge label Nov 23, 2017

wilzbach added the Merge:auto-merge label Dec 1, 2017

wilzbach merged commit 60fba57 into dlang:master Dec 2, 2017

wilzbach deleted the algo-skipover branch December 2, 2017 13:32

Uh oh!

Conversation

wilzbach commented Jul 8, 2017

Uh oh!

dlang-bot commented Jul 8, 2017

Bugzilla references

Uh oh!

wilzbach Jul 8, 2017

Choose a reason for hiding this comment

Uh oh!

wilzbach Jul 8, 2017

Choose a reason for hiding this comment

Uh oh!

andralex Jul 9, 2017

Choose a reason for hiding this comment

Uh oh!

wilzbach Jul 9, 2017

Choose a reason for hiding this comment

Uh oh!

MetaLang commented Jul 9, 2017

Uh oh!

wilzbach commented Jul 10, 2017

Uh oh!

MetaLang commented Jul 10, 2017

Uh oh!

CyberShadow commented Jul 10, 2017

Uh oh!

wilzbach commented Jul 10, 2017

Uh oh!

wilzbach commented Jul 17, 2017

Uh oh!

andralex commented Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

andralex Jul 25, 2017

Choose a reason for hiding this comment

Uh oh!

andralex Jul 25, 2017

Choose a reason for hiding this comment

Uh oh!

andralex commented Aug 11, 2017

Uh oh!

wilzbach commented Aug 11, 2017

Uh oh!

wilzbach commented Nov 17, 2017

Uh oh!

WalterBright commented Nov 23, 2017

Uh oh!

WalterBright commented Nov 23, 2017

Uh oh!

wilzbach commented Dec 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

andralex commented Jul 25, 2017 •

edited

Loading