Make iteration pattern consistent

[kamalmarhubi]

Currently there are two patterns of iteration used across the library:

• implementing the Iterator trait with Item=ParseResult (eg, DebugInfo::units / UnitHeadersIter, and DebugTypes::units / TypeUnitHeadersIter
• having a BlahIter type that does not implement Iterator, and instead has a next method that returns a ParseResult<Option<Blah>>.

From https://github.com/fitzgen/gimli/pull/37#issuecomment-239650372 it seems that the preferred iteration method is the second one.

I'm mostly interested in consistency, but will say that while I get the argument in the linked comment, I still prefer using Iterator<Item=ParseResult<Blah>>. In addition to the more ergonomic for _ in syntax, it also gets you the ability to do things like:

let units: Vec<_> = try!(debug_info.units().collect());

which is much more straightforward than

let aranges = vec![];
let mut iter = debug_aranges.aranges();
while let Some(arange) = try!(iter.next_arange()) {
    aranges.push(arange);
}

The potential for infinite loops on error can be fixed by having a done flag in the iterator struct, which gets set on the first error, or on successful completion of iteration. This is done in the tar crate, for example: https://github.com/alexcrichton/tar-rs/blob/master/src/archive.rs#L442.

[philipc]

The two patterns are:

• next() method that returns Option<ParseResult<_>> (a 'normal' iterator). This implements the Iterator trait.
• next() method that returns ParseResult<Option<_>> (a 'fallible' iterator). This can't implement the Iterator trait, but it can implement the FallibleIterator trait from https://github.com/sfackler/rust-fallible-iterator.

I agree that we're currently inconsistent in our usage of this. We started with normal iterators, and have been moving towards fallible iterators.

We want to make the API easy to use. There's two aspects to this though. It must be easy to use correctly, and hard to use incorrectly. There may be tradeoffs between these. My feeling is that fallible iterators are the better tradeoff.

In addition to the more ergonomic for _ in syntax

The for loop comparison is between these:

for unit in debug_info.units() {
    let unit = try!(unit);
   ...
}

and

let mut units = debug_info.units();
while let Some(unit) = try!(units.next()) {
   ...
}

Normal iterators are easier to use here, but I don't think the difference is that great.

it also gets you the ability to do things like: let units: Vec<_> = try!(debug_info.units().collect());

I don't think that works with normal iterators (I tried to figure out how you would do it, but couldn't... eg if I change one of the benchmarks to use that instead then it doesn't compile, but please correct me if I am wrong). The code you give does work with fallible iterators though if you use the FallibleIterator trait, so they are better in this regard. We haven't added a dependency on the fallible_iterator crate yet, but it would be easy to do. I was waiting for a real need to do so first.

The potential for infinite loops on error can be fixed by having a done flag in the iterator struct, which gets set on the first error, or on successful completion of iteration.

For most of our iterators we could easily do this by setting the input member to &[] on error, and we initially did this, but it did cause some bugs (see #21) which is what triggered the move to fallible iterators. This is what I mean when I say it needs to be hard to use incorrectly.

[kamalmarhubi]

I don't think that works with normal iterators (I tried to figure out how you would do it, but couldn't... eg if I change one of the benchmarks to use that instead then it doesn't compile, but please correct me if I am wrong).

It's definitely possible to go from Iterator<Item=Result<T, E>> to Result<Vec<T>, E> with collect: https://is.gd/CW0MON

But either approach makes sense to me. The fallible iterator crate looks pretty nice. :-)

[fitzgen]

FWIW, https://doc.rust-lang.org/nightly/std/iter/trait.FromIterator.html:

impl<A, E, V> FromIterator<Result<A, E>> for Result<V, E> where V: FromIterator<A>

---

I am leaning with @philipc on this one: with FallibleIterator, ergonomics are competitive with regular Iterator, and we remove one more potential footgun from the equation (that isn't just theoretical: I did already make this mistake!)

Concrete things to do in this issue:

• Move all of the remaining fn next(&mut self) -> Option<ParseResult<T>> over to fn next(&mut self) -> ParseResult<Option<T>>.
• Add fallible_iterator as a dependency, make all of our iterators actually impl FallibleIterator so that we actually realize the competitive ergonomics.