[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