maidsafe / lru_time_cache

LRU cache settable via size or time to live
BSD 3-Clause "New" or "Revised" License
104 stars 46 forks source link

impl `Index` and `IndexMut` for make it easy to use #81

Closed loggerhead closed 8 years ago

loggerhead commented 8 years ago

It would be convenient if LruCache can be indexed, like:

let mut cache: LruCache<String, String> = LruCache::with_capacity(1);
cache["foo".to_string()] = "bar".to_string();
loggerhead commented 8 years ago

To implement Index, it need to wrap list field in LruCache struct with RefCell. And I'm not suggest to implement IndexMut for avoiding misuse or misunderstand (spec https://github.com/rust-lang/rust/issues/23448).

afck commented 8 years ago

You're right, IndexMut doesn't make sense at the moment. But why would list need to be in a RefCell for Index? Shouldn't this basically just be self.get(key).unwrap()?

loggerhead commented 8 years ago

Because update_key used in get need &mut self, but Index need &self instead. I don't consider this carefully but use RefCell can avoid this kind of conflict in general.

afck commented 8 years ago

Ah, right! Yes, that's a bigger issue in itself: I think it might make sense to put that in a RefCell and remove the mut from all methods that don't change anything but timestamps. I'm unsure whether that would be a good use or abuse of inner mutability. @dirvine, @Fraser999: I think last time we discussed this we decided against it?

loggerhead commented 8 years ago

I don't think to remove the mut from all methods is a good idea, because it will give the user an wrong illusion that the cache keep unchanged. But I think use it only for Index or some private methods is acceptable.

afck commented 8 years ago

But isn't that inconsistent? For the cache's user it should either be clear that timestamp changes are or are not considered a mutation. If they are, we should not implement Index at all. If they are not, we should remove mut from the methods that mutate only timestamps, I think.

Fraser999 commented 8 years ago

Yes, I think we agreed to not do this as we felt it was as abuse - (we don't just mutate the list member in get() but also the map when we prune expired entries).

loggerhead commented 8 years ago

@afck Yes, it is inconsistent in some degree, like mutable or not. But it is also consistent with HashMap in standard library, and it is convenient to use. On the other hand, something like cache['k'] will hint the user it will call get method, if get method is mut, then the user will get the point.

afck commented 8 years ago

I find it misleading: As a user, I'd notice that get is mut and index is not, so I would assume that cache['k'] will not update any timestamps.

We could add an Index implementation that actually doesn't update timestamps, of course, but that also feels wrong for an LRU cache.

loggerhead commented 8 years ago

@afck Yes, you are right. Maybe we should wait for IndexSet ready 😃