[webkit-reviews] review denied: [Bug 67823] [NRWT] Provides a simple LRU cache class in Python. : [Attachment 107615] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Ai Makabi <makabi at google.com>'s
request for review:
Bug 67823: [NRWT] Provides a simple LRU cache class in Python.
https://bugs.webkit.org/show_bug.cgi?id=67823

Attachment 107615: Patch
https://bugs.webkit.org/attachment.cgi?id=107615&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=107615&action=review


FWIW, I think this would be less confusing if you just kept a list of keys
rather than the linked list.  As long as the capacity is small (maybe less than
20 entries), it'll still be plenty fast to update.  But this approach is fine
too if you fix the memory leak I mentioned.

> Tools/Scripts/webkitpy/common/lru_cache.py:39
> +    Args:
> +	   capacity: a cache memory capacity

Nit: This should be in the docstring for __init__.

> Tools/Scripts/webkitpy/common/lru_cache.py:57
> +	       if self._capacity == 1:
> +		   self._one_node(key, value)

I think you need to clear self._first here or the new node will point to the
old value.  If the capacity is 1, we leak all the old objects because we hold a
ref to them.