return error result only from Iterator.next()

[mykmelez]

This is an alternative to #13 that minimizes the number of methods that return an error result down to just the Iterator.next() implementation in Iter by using std::iter::once() to create an iterator that returns an error result when a failure occurs in the Cursor.iter*() methods.

A potential downside of this approach is that it returns an Iterator trait object from the Cursor.iter*() methods, which could have runtime performance implications in theory (although I'm unsure that it does in practice).

(I've previously requested integration of these changes upstream in danburkert#45 and would like to move forward with these changes downstream while awaiting that integration.)

[mykmelez]

@ncalexan I've submitted this separately from #13 and requested review from you for both branches because I'd like your input on the tradeoff between the ergonomic benefits of minimizing the number of fallible methods vs. the potential performance (or other) implications of using a trait object in this branch.

(Also happy for any insight you have into another way we could make the Cursor.iter*() methods infallible without using a trait object.)

[ncalexan]

I didn't review this thoroughly, 'cuz I want to talk through options. It's weird to me that any of these iterators can produce errors. iter_dup_of with an unknown key isn't an error; it should always yield an empty iterator. iter_from shouldn't error if the value isn't found, it should produce an empty iterator. And if these things for reasons I don't understand do error, it should be on the use of the iterator (.next()), not at creation time. (Almost certainly.)

Let's talk about this more soon.

[ncalexan]

The way to avoid the Box is easy enough -- push the branch down into the underlying Iter struct. That is, make it:

/// An iterator over the values in an LMDB database.
pub enum Iter<'txn> {
    Err(Result::E),
    Ok(...),
}

where the bits that are currently in Iter are in the Ok branch as well.

[mykmelez]

The way to avoid the Box is easy enough -- push the branch down into the underlying Iter struct.

Ok, I've done this in a558fab.

[ncalexan]

Here, I see how you will only get an error once (and then the iterator ends). With the other approach (Item = Result<...>), how do you ensure you don't get Err forever? This feels similar to fuse in spirit.

[mykmelez]

Here, I see how you will only get an error once (and then the iterator ends). With the other approach (Item = Result<...>), how do you ensure you don't get Err forever? This feels similar to fuse in spirit.

I don't currently ensure that; rather, I leave it to the caller to decide what to do on error. I'm not entirely sure if it's possible for LMDB to return a value after returning an error. If so, then consumers should be able to continue iterating in that case.

Otherwise, consumers that call Iterator.next() directly can easily stop iterating at the first error, since they need to match on the return value anyway to distinguish between the Some(Ok), Some(Err) and None types; and collections can easily automatically stop collecting at the first error; so it seems like consumers wouldn't gain much by the Iter returning None after that.

[mykmelez]

I didn't review this thoroughly, 'cuz I want to talk through options. … Let's talk about this more soon.

Does this implementation match your expectations based on the conversation we had?

[mykmelez]

Travis failed only on Rust 1.20.0 with errors like:

error[E0308]: mismatched types
   --> src/cursor.rs:265:13
    |
265 |             Iter::Ok { cursor, ref mut op, next_op, _marker } => {
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected mutable reference, found enum `cursor::Iter`
    |
    = note: expected type `&mut cursor::Iter<'txn>`
               found type `cursor::Iter<'_>`

I'm unsure what to do about that. Perhaps requiring newer Rust is the simplest solution.

[mykmelez]

I'm unsure what to do about that. Perhaps requiring newer Rust is the simplest solution.

af923dc fixes Rust 1.20.0+ by explicitly identifying references in the match patterns.

[mykmelez]

@ncalexan Does this look good with the latest set of changes in response to your review comments?

[ncalexan]

OK, this makes much more sense to me now! Sorry for delayed review.

(I reviewed the squashed diff 'cuz there was a lot of inter-commit churn; I expect that's okay in this situation.)