feat(ffi-iterator): Implementation of Iterator in Go (2/4) #1256
Conversation
AminR443
requested review from
rkuris,
demosdemon and
alarso16
as code owners
|
I do think there's some test cases missing. What if you're using |
| func (db *Database) LatestRevision() (*Revision, error) { | ||
| root, err := db.Root() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if bytes.Equal(root, EmptyRoot) { | ||
| return nil, errRevisionNotFound | ||
| } | ||
| return db.Revision(root) | ||
| } |
There was a problem hiding this comment.
This is racy. If a commit occurs after line 218 and before line 225, you won't have the latest revision.
There was a problem hiding this comment.
It seems like the user's responsibility to ensure that there isn't a commit occurring.
| i := 0 | ||
| for ; it.Next(); i += 1 { | ||
| r.Equal(keys[i], it.Key()) | ||
| r.Equal(vals[i], it.Value()) | ||
| } |
There was a problem hiding this comment.
Why did we decide not to use go iterators and range here?
| } | ||
|
|
||
| // Value returns the value of the current pair | ||
| func (it *Iterator) Value() []byte { |
There was a problem hiding this comment.
Is there ever a case where the caller will only be interested in the key or the value (not both)? If not, then it seems like having a method called KeyValue would be a lot more usable, which returns (key []byte, value []byte)
| let out = self.iterator.as_mut()?.next(); | ||
| if out.is_none() { | ||
| // iterator exhausted; drop it so the NodeStore can be released | ||
| self.iterator.take(); |
There was a problem hiding this comment.
why take?
This should be:
| self.iterator.take(); | |
| self.iterator = None; |
This implement a simple Iterator interface in Go (
Next,Key,Value) using the introduced FFI functions, without batching.Depends On: #1255