Bump dependency on lru to from version 0.7.5 to version 0.9.0. #1755
Conversation
|
Thank you! |
|
Thank you! @adamreichold. |
evanxg852000
dismissed
their stale review
I am just not keen on wrapping the cache in Option<...>
| cache: Mutex<LruCache<usize, Block>>, | ||
| cache_hits: Arc<AtomicUsize>, | ||
| cache_misses: Arc<AtomicUsize>, | ||
| cache: Option<Mutex<LruCache<usize, Block>>>, |
There was a problem hiding this comment.
I believe this option is just for being able to use NonZeroUsize. I think it's simpler to stick to cache: Mutex<LruCache<usize, Block>>. If we want to forbid cache size of zero, an assert can be appropriate.
There was a problem hiding this comment.
I actually started by propagating the use of NonZeroUsize through to the code base, but there are places where, indirectly, zero-size caches are created. To continue to support this, I chose the Option<..> layer which is not so much about using NonZeroUsize but more about supporting zero-sized caches which lru does just not support any more.
Personally, I see two viable options:
- Using
Option<..>to continue supporting zero-sized caches. - Propagate the
usize -> NonZeroUsizetype change throughout the API and change any callers to use at least cache size one.
Just asserting a non-zero cache might lead to hard to diagnose panics IMHO as there are multiple layers of indirection between constructing readers, writers, etc. and the usage of LruCache.
There was a problem hiding this comment.
Ok, to keep the change in scope let's stick with Option<...> then.
There was a problem hiding this comment.
I have the second alternative ready in a branch. So if you want that, I can propose that as a follow-up PR since I do agree that having the local change first is much easier to review.
No description provided.