search

contain-rs / lru-cache

A cache that holds a limited number of key-value pairs
https://contain-rs.github.io/lru-cache/lru_cache/
Apache License 2.0
82 stars 36 forks source link

contains_key #17

Closed xrl closed 9 years ago

xrl commented 9 years ago

I want to make LruCache a drop in replacement for my needs, this was the only feature missing.

FlashCat commented 9 years ago

Thanks for the pull request, and welcome! The contain-rs team is excited to review your changes, and you should hear from @huonw (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

apasel422 commented 9 years ago

Thanks!

Can you make the signature of this method

fn contains_key<Q: ?Sized>(&self, key: &Q) -> bool
    where K: Borrow<Q>, Q: Eq + Hash

in order to match get_mut?

Also, it's not clear to me whether this should affect the LRU state of the key. @Gankro can weigh in on that, but in any case, the behavior in that regard should be documented (see the iter method for an example).

xrl commented 9 years ago

Sure thing, changes pushed up.

xrl commented 9 years ago

One thing, I'm not sure why the documentation code with a number literal needed a & to pass. Any ideas? I'm kind of lost on the purpose of the type sigs.

apasel422 commented 9 years ago

Thanks. I'm just waiting to hear about the LRU stuff.

As for the need for &: It's because contains_key's key argument is of type &Q, which means it's a reference to some type Q. In this case, because the cache is storing integer (i32) keys, contains_key expects an argument that can be borrowed to &i32. The integer literal 1 by itself is of type i32, which is not a reference type, so the & operator is needed to get an &i32. The guide has more info.

xrl commented 9 years ago

Cool, thanks for the explanation. That makes sense.

Gankra commented 9 years ago

I would assume contains_key should have the same behaviour as get since it's always a trivial wrapper?

apasel422 commented 9 years ago

@Gankro I agree, though that requires contains_key to take &mut self and defer to self.get_mut() (which is fine).

xrl commented 9 years ago

What do I need to do to make this useful for merging?

apasel422 commented 9 years ago

@xrl Sorry for the delay. Can you change the contains_key method to take &mut self and reimplement it as self.get_mut(key).is_some()? Once you do that and squash the commits, it should be ready to merge. Thanks!

xrl commented 9 years ago

Bingo-bango. Take a look now.

And please push this version up to crates so I can do my own release. Thanks!

apasel422 commented 9 years ago

Thanks!