[Webkit-unassigned] [Bug 67823] [NRWT] Provides a simple LRU cache class in Python.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Sep 15 22:27:49 PDT 2011
https://bugs.webkit.org/show_bug.cgi?id=67823
--- Comment #11 from Ai Makabi <makabi at google.com> 2011-09-15 22:27:49 PST ---
I addressed your comment.
(In reply to comment #9)
> (From update of attachment 107582 [details])
> 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.
More information about the webkit-unassigned
mailing list