Skip to content

Reducers#1102

Merged
josevalim merged 6 commits intoelixir-lang:masterfrom
ericmj:reducers
May 23, 2013
Merged

Reducers#1102
josevalim merged 6 commits intoelixir-lang:masterfrom
ericmj:reducers

Conversation

@ericmj
Copy link
Copy Markdown
Member

@ericmj ericmj commented May 22, 2013

No description provided.

ericmj added 2 commits May 22, 2013 15:02
Remove Enum.equal?
Remove File.iterator/2 and File.biniterator/2
Comment thread lib/elixir/lib/enum.ex Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since both this clause and the next clause are basically doing a reduction, I think we can get rid of this one altogether.

Comment thread lib/elixir/lib/enum.ex Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This one is wrong. Basically, after the first time the function returns false, we get the rest of the list. It should be something like:

fn
  (entry, []) -> if fun.(entry), do: [], else: [entry]
  (entry, acc) -> [entry|acc]
end

We are basically using the accumulator to check if something was added so far (or not).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch, will add tests for it aswell.

@josevalim
Copy link
Copy Markdown
Member

❤️ 💚 💙 💛 💜

Comment thread lib/elixir/lib/enum.ex Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Replace nil by true here and in the next line, and we can get rid of the true return value before catch.

@josevalim
Copy link
Copy Markdown
Member

I have seen that you kept the specific clauses for lists in many functions but we can get rid of many of them. We definitely should remove: Enum.count/2, Enum.join/2, Enum.map_join/3, Enum.partition/2, Enum.map_reduce/3, Enum.reduce/2 as the reducer clause will provide the same performance benefits.

We should also rewrite Enum.max(list), Enum.min(list) to use reducers instead of :lists.max(list) and :lists.min(list) (I want to avoid dependency on :lists which is not tail recursive).

We should consider removing the specific lists clauses for Enum.all?/2, Enum.any?/2, Enum.fetch/2, Enum.find_index/2, Enum.find/3 and Enum.find_value/3 too. The only extra overhead is a throw. But let's not worry about it now, we can always discuss those later.

@ericmj
Copy link
Copy Markdown
Member Author

ericmj commented May 22, 2013

@josevalim New commit with discussed changes.

@josevalim
Copy link
Copy Markdown
Member

There are two things pending I believe:

  1. Should we still call the protocol Enum.Iterator? We could rename it to Enumerable, Enum.Reducible, Enum.Collection, etc
  2. Docs for reduce in the protocol

/cc @alco @devinus @yrashk @ericmj please tell me your thoughts

@ericmj
Copy link
Copy Markdown
Member Author

ericmj commented May 22, 2013

I think I prefer Enum.Reducible. I'm working on the docs.

@meh
Copy link
Copy Markdown
Contributor

meh commented May 22, 2013

I'd call it Enumerable, the point of it is clearer, Enum.Reducible doesn't sound clear at all.

@ericmj
Copy link
Copy Markdown
Member Author

ericmj commented May 22, 2013

I'd call it Enumerable, the point of it is clearer, Enum.Reducible doesn't sound clear at all.

It is simply something that is reducible with the reduce function. Although, we also supply the functions member? and count so Reducible might not be the perfect name.

@alco
Copy link
Copy Markdown
Member

alco commented May 22, 2013

I'd like to see how the docs will describe this new stuff before deciding on the name.

@ericmj
Copy link
Copy Markdown
Member Author

ericmj commented May 22, 2013

Added documentation

@alco
Copy link
Copy Markdown
Member

alco commented May 22, 2013

It would be nice to have some historical background of the reasons for migrating from iterator to reduce. Not necessarily in the docs, it could be mentioned in a commit message.

Since the main point of the protocol is reducing a collection, I'd call it Enum.Reducer. A collection is called reducible if it implements Reducer, i.e. it is compatible with Enum functions.

Enumerable doesn't quite fit -- it implies enumeration of a collection. But Enum doesn't simply enumerate, it produces something new (a list) for the given collection. Therefore, Reducer is sufficiently descriptive in my view -- we'd call a reducer something that helps transform the collection by means of the reduce function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants