[Webkit-unassigned] [Bug 67823] [NRWT] Provides a simple LRU cache class in Python.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=67823





--- Comment #9 from Hayato Ito <hayato at chromium.org>  2011-09-15 21:34:52 PST ---
(From update of attachment 107582)
Thank you for addressing my comments! I added some comments. Sorry for the back and forth again.
I hope you will enjoy your last day of the internship. :)

View in context: https://bugs.webkit.org/attachment.cgi?id=107582&action=review

> Tools/Scripts/webkitpy/common/lru_cache.py:59
> +        node = Node(key, value, self._first)

I think if self._dict already contains the key, we have to delete the node with the key from the doubly linked list.
Without that, we are likely to have two nodes with the same key in the doubly liked list.

> Tools/Scripts/webkitpy/common/lru_cache.py:77
> +            next__last = self._last.next

Nit: Two underscores '__'.
You meant |next_last|, |next_first|?

> Tools/Scripts/webkitpy/common/lru_cache.py:88
> +        key_next = self._dict[key].next

You might use |node| here.
  key_next = node.next
  key_prev = node.prev

Or You might rewrite L88-L91 with the following two lines:
  node.next.prev = node.prev
  node.prev.next = node.next

> Tools/Scripts/webkitpy/common/lru_cache.py:99
> +        if self._first == self._last:

It seems that this should be 'self._first is self._last'.

> Tools/Scripts/webkitpy/common/lru_cache.py:102
> +            del self._dict[key]

It seems that if the key is not found here, a lru_cache will become inconsistant state (self._dict has one item, but self._first is None).

It might be better to call 'node = self._dict[key]' at the beginning of the function.
e.g.

   def __delitem__(self, key):
       node = self._dict[key]
       del self._dict[key]
       if self._first is self._last:
            self._first = None
            self._last = None
            return
       if self._first is node:
            self._first = node.prev
            self._first.next = None
            return
       if self._last is node:
            self._last = node.next
            self._last.prev = None
            return
       node.next.prev = node.prev
       node.prev.next = node.next

I've not tested this. I guess your existing unittests (or new tests) will find any bugs in this code :)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.