danburkert / lmdb-rs

Safe Rust bindings for LMDB
Apache License 2.0
172 stars 87 forks source link

return error result only from Iterator.next() #45

Closed mykmelez closed 5 years ago

mykmelez commented 6 years ago

This is an alternative to #44 that minimizes the number of methods that return an error result down to (almost, see below) 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.

The only exception is Cursor.iter_dup_from(), which still returns an error result because I encountered a compiler panic while trying to convert it to return an iterator that returns an error result. (I'm still looking at this to figure out if it's possible to work around the failure.)

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).

mykmelez commented 6 years ago

The only exception is Cursor.iter_dup_from(), which still returns an error result because I encountered a compiler panic while trying to convert it to return an iterator that returns an error result. (I'm still looking at this to figure out if it's possible to work around the failure.)

I worked around it in 58d46ec and converted Cursor.iter_dup_from() to return a boxed iterator as well, so now all Cursor.iter*() methods do so, and only Iter.next() returns a result.

mykmelez commented 5 years ago

@danburkert Recent commits on this branch included unrelated changes, so I've reset the branch to a commit that contains just this change. I also modified it, based on feedback from a colleague, to use an enum to distinguish between success and failure to retrieve an iterator, which means it no longer uses a Trait object to represent the Iter, so it shouldn't incur a runtime performance penalty.

mykmelez commented 5 years ago

The build bustage is unrelated and fixed by #47.

mykmelez commented 5 years ago

Closing this in favor of the approach in #48.