use standard iterator for Stack + Queue Fixes #116

[rawls238]

@kmsquire

This moves the iterator for stack and queue to conform to iterators such as those in https://github.com/JuliaLang/Iterators.jl

It effectively just calls down and utilizes the iterator for Deque

[kmsquire]

Okay, seems mostly reasonable to me.

Because the iter interface was released (even if only for a short time), it should probably go through a period of deprecation. I'm not sure the standard deprecation interface will work, here. Try it (see #161), and if it doesn't work, print a warning using Base.warn_once.

If you haven't yet, look at the discussion in #116 (which probably could be closed). That's discussion is the reason that the interface is the way it is now. I think if we're going to go the route suggested there, it should be planned out better (iter doesn't seem like the best name to me). So I'm inclined to do this first, which is simpler--additional iterators can always be added later.

Cc: @lindahua @RaviMohan

[kmsquire]

(And of course, you noted that this fixes #116 in this PR, so you obviously saw that...)

Anyway, thanks for the PR!

[rawls238]

@kmsquire I saw the discussion there, though I think I was wrong in removing the iter method here after re-reading and rethinking. I think we should encapsulate the default iterator behavior by defaulting to the standard Julia iterator interface (and have an implementation that doesn't make a copy of the underlying object), but for both of these types allows a forward_iter and a backward_iter method which follows the same interface as the iter method. Thoughts on that approach?

[kmsquire]

I think that sounds good. I wouldn't copy the iter method though. Instead I would create immutable ForwardDequeIterator and BackwardDequeIterator types and use these for iterating Stacks, Queues, and Deques. That would be cleaner than copying the underlying data that iter is doing now.

Iterators.jl mostly uses this paradigm.

[rawls238]

@kmsquire let me know what you think of this - I ended up using Tasks to implement the forward and backward iters + made the default iterator for stack be LIFO and the default iterator for queue be FIFO

[kmsquire]

Hi @rawls238, unfortunately, while it seems elegant, using Tasks is probably not the way to go. The quickest showstopper is

julia> y
DataStructures.Stack{Int64}(Deque [[28,83,82,77,61,53,17,13,73,51]])

julia> [i for i in forward_iter(y)]
ERROR: MethodError: `length` has no method matching length(::Task)
 in anonymous at no file

Since a Task is not parameterized by what it wraps, there's no way to define that function either.

I'll add a few other comments inline.

[kmsquire]

In other areas of julia, reverse is used instead of backward. While backward isn't wrong, it would be nice to be consistent.

[kmsquire]

Although now that I say that, none of these is particularly clear for stacks, at least (because forward or backward/reverse depends on the implementation).

[rawls238]

see the discussion in the corresponding issue. I was using backward_iter as the name from the discussion there, happy to change to reverse_iter.

Currently, the default iterator uses LIFO order for stacks and FIFO for queues, whereas forward_iter is always FIFO and backward_iter is always LIFO. I think it makes sense for the default iterator to use the ordering that makes sense for the particular data structure whereas the separate functions for forward_iter and backward_iter should be consistent across data structures. Let me know what you think about that

[kmsquire]

@lindahua, who introduced those names, is coming from a C++ background, and in C++, the corresponding iterators are forward_iterator and backward_iterator.

Thinking more, I would actually prefer that, instead, we had a natural order iterator (the default), and a second, reverse_iter, which is just opposite of the default iteration. Reasons:

1. It's easier to reason about--you don't have to understand how the underlying data structure is implemented, you only have to understand its default iteration order.
2. At one point, a generic reverse iterator (and corresponding Reverse type) was implemented this way in Julia (it was later changed to return a concrete value).
3. It's one less function to create.

[rawls238]

I modified things such that there are is a DequeIterator and a ReverseDequeIterator but the default iteration order is the DequeIterator for the deque object (i.e. if you just do

for i in dq
  println(i)
end

it iterates in the order you expect it to). I still think it makes sense to define a forward_iter method which can optionally be used to get objects in a FIFO ordering and a reverse_iter method which can optionally be used to get objects in a LIFO ordering. Deque, Stack, and Queue have their default iteration order defined as FIFO, LIFO, and FIFO respectively and in most cases people should just use their default iteration order (perhaps to make this clearer we should have the iter function which returns the default iteration order for the particular type - generally I think most people would use the conventional iterator syntax for i in q as opposed to for i in forward_iter(q)), but if you particularly want things in LIFO order then you would use reverse_iter (and vice versa for FIFO order).

Let me know if you think that's simply overcomplicating things. Also, regardless of the direction we take, we should document this behavior clearly.

[rawls238]

actually I like the idea of leaving iter but having it return the default iterator for the type

[kmsquire]

Not going to have time to look at this closer right now, and the implementation is going to take some thought. I'll try to write more later.

[rawls238]

hm, good point around why Tasks won't be ideal here. Going to think of how to do this in an alternative way - let me know if you have any ideas

[kmsquire]

q.head.back should be q.rear.back, right?

[kmsquire]

@rawls238, check out #177. This would be my preference for Deque iteration, and it might give you some ideas for the rest of this PR. (Comments welcome in #177!)

[rawls238]

thanks @kmsquire, I built off your work there and made some changes in here. Will make some comments inline

[rawls238]

@kmsquire I also applied your cleanups to the existing iterator

[kmsquire]

Nice update!

My preference, again, is to remove forward_iter, and make reverse_iter always produce the opposite of the default iterator.

So, for example, the default Stack iterator would go from the end of storage toward the beginning, and reverse_iter would produce the reverse of the default sequence (which, in this case, would go from the beginning of the underlying storage toward the end). (This is the opposite of what you have now.)

The main idea is that reverse is not defined in terms of the underlying storage, but in terms of the expected behavior of the iterator.

I also think that iter should actually be deprecated.

@DanielArndt @lindahua any thoughts/opinion on this?

[kmsquire]

@rawls238, in my email, I see a couple of other responses from you (where you argue the opposite point of view), but I don't see those comments here. Can you point them out, or repost them?

[kmsquire]

Cc: @StefanKarpinski, who might have an opinion about reverse iteration.

[DanielArndt]

@kmsquire Are you looking for this discussion? #171 (comment)

[DanielArndt]

@kmsquire Re: iterators, I agree that the default iterator and reverse_iter should semantically make sense, and therefore reverse_iter should in fact iterate the default iterator in reverse.

It seems natural to me that the default iterator would iterate according to what you expect from the of the data structure, as you suggested.

That being said, I am curious if there would be value in having a way to specifically specify that you would like a FIFO or FILO iterator which is, I think, what @rawls238 is suggesting. It definitely seems like a much less common use case.

edit: I typed iter, but my brain was thinking default iterator.

[rawls238]

Yes that's what I'm suggesting, but I can see the appeal in just having the default iterator and reverse iterator since that simplifies things versus the approach I decided to take and makes it so that you don't have to think about the internals of stack and queue in order to decide what iterator to use (you just need to know the default iteration order for the particular data structure which should be documented). Unless anyone else thinks having explicit LIFO / FIFO iterators is a good idea, I'll change this to follow the default / reverse pattern.

[rawls238]

@kmsquire decided to just switch this to the interface you described. Also added a blurb in the docs about the iterators, let me know if you think this is good to go

[kmsquire]

I've always seen FILO written as LIFO...

[DanielArndt]

That's my fault! I started mix matching a few things yesterday. A good sign that one needs more sleep.

[rawls238]

heh yea I also thought this was strange, but the docs already were using FILO so I figured I would follow them here. I'll switch everything in this file to LIFO

[kmsquire]

Other than those comments, LGTM.

[rawls238]

thanks for reviewing this @kmsquire, @DanielArndt