[DONTMERGE] Use iterator for keys #57
Conversation
|
|
||
| if rewind { | ||
| self.rewind(); | ||
| other.rewind(); |
There was a problem hiding this comment.
@Schaeff because the calling code expects the next call to one iterator to return the first differing byte in the key, I find myself having to rewind the iterator by one byte. I have tried to use Peekable but it will also advance the iterator underneath. Any suggestion here?
There was a problem hiding this comment.
Then the comment at line 67 is incorrect as you're leaving them at their last common element?
From Peekable it seems that your implementation of the trait was incorrect as it's not supposed to advance the iterator, maybe an issue there?
There was a problem hiding this comment.
The comment is indeed too vague. The thing is: Peekable is an iterator, that doesn't advance on peek. But the iterator that it consumes does advance. It's not a problem with just this iterator, it's the problem for all of them:
fn main() {
let v = vec![1, 2, 3];
let mut pit = v.iter().peekable();
println!("before peek {:?}", pit);
pit.peek();
println!("after peek {:?}", pit);
}output:
before peek Peekable { iter: Iter([1, 2, 3]), peeked: None }
after peek Peekable { iter: Iter([2, 3]), peeked: Some(Some(1)) }
There was a problem hiding this comment.
Also, using Peekable isn't great for another reason: since it's not defined in the same package as KeyIterator, I wouldn't be able to perform the trick that saves me a copy 😞
There was a problem hiding this comment.
Okay my bad it looks more complicated than that.
| @@ -152,6 +135,7 @@ impl std::ops::Index<std::ops::RangeFrom<usize>> for NibbleKey { | |||
| impl std::ops::Index<std::ops::RangeTo<usize>> for NibbleKey { | |||
| type Output = [u8]; | |||
|
|
|||
| // Problem here | |||
There was a problem hiding this comment.
@Scaeff this is the second major problem: I would like to implement the iterator as just going over the key's bytes and extracting either the nibble or bit. But since I have to return &Self::Output, I find myself returning the whole byte instead of just the bit/nibble. Any idea?
There was a problem hiding this comment.
If you don't want [u8], what happens if you set Self::Output to something else?
There was a problem hiding this comment.
I still need to pass a reference to something, and that means I would have to create it on the fly, which isn't possible as it would go out of scope. I could somehow try to create a temporary element in a Vec and return a ref to it as a slice, however I'm not sure exactly how it would work and also I would not returning a reference to the underlying structure, so I would require some form of allocation. Not a show stopper, but not great either.
| fn is_empty(&self) -> bool; | ||
|
|
||
| /// Returns an iterator over the key components (bit, byte, etc...) | ||
| fn iter(&self) -> KeyIterator<T, Self> |
There was a problem hiding this comment.
How about using IntoIterator as a supertrait (just heard the term to be honest..)
There was a problem hiding this comment.
because it moves the values out of the container, but maybe I can fiddle so that it doesn't... 🤔
| nibbles.push((address.0[nibble / 2] >> nibble_shift) & 0xF); | ||
| } | ||
| NibbleKey(nibbles) | ||
| impl<'a> From<KeyIterator<'a, u8, NibbleKey>> for NibbleKey { |
There was a problem hiding this comment.
How about using FromIterator here? Then we can use the more specific collect method when consuming the iterator, instead of the generic into
There was a problem hiding this comment.
yeah that sounds like a good idea
| @@ -152,6 +135,7 @@ impl std::ops::Index<std::ops::RangeFrom<usize>> for NibbleKey { | |||
| impl std::ops::Index<std::ops::RangeTo<usize>> for NibbleKey { | |||
| type Output = [u8]; | |||
|
|
|||
| // Problem here | |||
There was a problem hiding this comment.
If you don't want [u8], what happens if you set Self::Output to something else?
|
|
||
| if rewind { | ||
| self.rewind(); | ||
| other.rewind(); |
There was a problem hiding this comment.
Then the comment at line 67 is incorrect as you're leaving them at their last common element?
From Peekable it seems that your implementation of the trait was incorrect as it's not supposed to advance the iterator, maybe an issue there?
yup |
This PR needs to be rebased, only applies iterators to
Extensionnodes and is only here for discussing the following points:Index'sindex -> &[u8]makes it impossible to make bit keys and nibble keys as simpleVec<u8>: the data needs to be expanded in anotherVec<u8>in which each byte represents one bit/nibblerewindchop_common