Skip to content

Conversation

atilaneves
Copy link
Contributor

I took the easy way out here; I have no idea why reduce throws when passed an empty range and I don't think fold should do it either. However, since it's a lot harder to make fold nothrow I'll leave it as it is right now and maybe submit a PR later to make it so. IMHO, this should compile and pass:

@safe pure nothrow unittest
{
    int[] arr;
    assert(arr.fold!((a, b) => a + b) == 0);
    assert(arr.fold!(min, max) == tuple(0, 0));
}

i.e. return Unqual!(ElementType!R).init when passed in an empty range. It gets more complicated since reduce also supports opApply.

@atilaneves atilaneves force-pushed the fold branch 2 times, most recently from c746e23 to 72eab7f Compare February 3, 2016 20:13
@MetaLang
Copy link
Member

MetaLang commented Feb 3, 2016

I have no idea why reduce throws when passed an empty range

I believe this is just historical baggage. Today it would be written as assert(!range.empty) or just left up to the range to assert when front/popFront are called and empty == true.

Lazily computes all permutations using Heap's algorithm.)
$(T2 reduce,
$(D reduce!((a, b) => a + b)([1, 2, 3, 4])) returns $(D 10).)
$(T2 fold,
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't break the lexical order.

@dcarp
Copy link
Contributor

dcarp commented Feb 5, 2016

Changelog entry is missing.

@dcarp
Copy link
Contributor

dcarp commented Feb 5, 2016

For a smoother deprecation path, it will be better to use fold in the original implementation and create reduce as a wrapper of fold.
Initially the unit tests can still use reduce. This will assure that the wrapper implementation works in all the tested cases.

@dcarp
Copy link
Contributor

dcarp commented Feb 5, 2016

The reference in std.algorithm.package.d's ddoc is missing.

@atilaneves
Copy link
Contributor Author

I don't think it's a good idea to use fold as the "real" implementation. It's a lot more work for little to no benefit, and contrary to what was discussed here.

assert(arr.fold!(min, max) == tuple(1, 5));

// Compute minimum and maximum at the same time with seeds
assert(arr.fold!(min, max)(0, 7) == tuple(0, 7));
Copy link
Member

Choose a reason for hiding this comment

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

What about adding an example of a UFCS chain, which is the primary motivation for introducing this function over reduce in the first place?

@quickfur
Copy link
Member

Looks like a rebase is in order.

@atilaneves
Copy link
Contributor Author

Added UFCS example and rebased.

assert(arr.fold!(min, max)(0, 7) == tuple(0, 7));

// Can be used in a UFCS chain
assert(arr.map!(a => a +1).fold!((a, b) => a + b) == 20);
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: insert space after +, i.e., + 1 instead of +1.

@quickfur
Copy link
Member

Also, please add fold to the ddoc index in std/algorithm/package.d.

@wilzbach
Copy link
Contributor

Nice! I came by this issue from #1955 and thought this will never make it into phobos. Thanks @atilaneves!

I would recommend to add some comments to reduce about fold - newbies like I am are otherwise confused how to call reduce in UCFS syntax with seeds ;-)

@atilaneves
Copy link
Contributor Author

I'm on holiday right now, will address the most recent comments when I get back home

@atilaneves
Copy link
Contributor Author

Comments addressed.

@quickfur
Copy link
Member

Thanks! LGTM.

@quickfur
Copy link
Member

ping random reviewers @JakobOvrum @andralex @yebblies @klickverbot
This has been green for a while. Any other comments that need to be addressed, or can we merge this now?



/++
Implements the homonym function (also known as $(D accumulate), $(D
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to use backticks instead of $(D ...) throughout (not a blocker).

@andralex
Copy link
Member

andralex commented Mar 3, 2016

This looks good. Will pull as is but please there is one follow-up task for @atilaneves: please make one more pass through all documentation and replace all uses of reduce with fold. Also deemphasize reduce to send people to fold. Essentially, even though fold is implemented in terms of reduce, the documentation and examples should go the other way around. Thanks!

@andralex
Copy link
Member

andralex commented Mar 3, 2016

Auto-merge toggled on

andralex added a commit that referenced this pull request Mar 3, 2016
Introduce "fold" as an alternative to "reduce"
@andralex andralex merged commit 6d6d8e2 into dlang:master Mar 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants