orlp / slotmap

Slotmap data structure for Rust
zlib License
1.08k stars 70 forks source link

Why are Keys / Values iterators generic over both Key and Value? #101

Open EriKWDev opened 1 year ago

EriKWDev commented 1 year ago

Before I make a PR to perhaps change this, is there any reason that the Keys and Values iterators have an inner iterator of both keys and values?

I'm looking at the implementation for DenseSlotMap's Keys and since it just ignores the value it seems like an extra unnecessary step + extra unnecessary code generation for the extra generic.

fn next(&mut self) -> Option<K> {
    self.inner.next().map(|(key, _)| key)
}

The only thing I see that could be an issue is that the standard Key/Value iterator only yields in case both the key and value exist at the same location, but I don't know if this is just because of the iterator type constraints or whether such a case could ever occur:

fn next(&mut self) -> Option<(K, V)> {
    let key = self.inner_keys.next();
    let value = self.inner_values.next();

    if let (Some(k), Some(v)) = (key, value) {
        Some((k, v))
    } else {
        None // Is this unreachable if either key or value is Some(_) ?
    }
}

If the reason for this is to make a Keys iterator be generic over the Value and vice verse that could be solved by a PhantomData

pub struct Keys<'a, K: 'a + Key, V> {
    inner_keys: core::slice::Iter<'a, K>,
    _value_marker: PhantomData<V>,
}

..but I don't really see why it is useful

orlp commented 1 year ago

It was done at the time for code reuse. I do plan on changing it, but am reluctant to accept pull requests before a major rewrite I've been working on for a while now.

orlp commented 8 months ago

Looking at it a bit closer, it's more than just code reuse. For SlotMap the iterator must iterate over slots, and to do that it must know the size of the value type.

Technically this is not necessary for the DenseSlotMap, but it makes porting code between the two more annoying if it didn't follow the same signatures.