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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Sep 14 06:27:34 PDT 2011


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





--- Comment #4 from Ai Makabi <makabi at google.com>  2011-09-14 06:27:34 PST ---
I addressed your comment.(In reply to comment #2)
> (From update of attachment 107297 [details])
> Ai-san, thank you for the patch.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=107297&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/reftests/lru.py:1
> > +#!/usr/bin/env python
> 
> I think this file should move to webkitpy/common/ or webkitpy/common/system directory.
> And lru_cache.py might be better file name than lru.py
> 
> > Tools/Scripts/webkitpy/layout_tests/reftests/lru.py:28
> > +class Node():
> 
> It might be better that Node class should have 'value' member in addition to 'prev' and 'next'.
> That would make the implementation of LRU class simpler because we don't have to have a tuple (value, Node(...)).
> 
> > Tools/Scripts/webkitpy/layout_tests/reftests/lru.py:36
> > +
> 
> L36-L37: One blank line instead of two blank lines.
> 
> > Tools/Scripts/webkitpy/layout_tests/reftests/lru.py:40
> > +    def __init__(self, size):
> 
> Could you add a blank line between Line39 and Line40?
> 
> > Tools/Scripts/webkitpy/layout_tests/reftests/lru.py:44
> > +        self._size = size
> 
> What if size <= 0? Could you assert the value of |size| here?
>   assert size > 0
> 
> > Tools/Scripts/webkitpy/layout_tests/reftests/lru.py:49
> > +            self.first = key
> 
> I think self.first and self.last should point to the first Node and the last Node directly.
> 
> def __setitem__(self, key, value):
>     if not self.first:
>         node = Node(value, ,,,)
>         self._dict[key] = node
>         self.first = node
>         self.last = node
>         self.last_key = key  // I am not sure, but I guess we should keep track of the last_key to delete the last Node from the self_dict.
> 
> And Node class's |prev| and |next| also point to other Nodes directly, instead of having |key|.
> That might make the implementation of this LRU class more natural.
> 
> That'd be nice if you could try to rewrite this class as I suggested.
> 
> > Tools/Scripts/webkitpy/layout_tests/reftests/lru.py:114
> > +    def pop(self, key):
> 
> Do we need this |pop| method?
> To look up a value by a key, 'lru[key]' is good enough because this class defines __getitems__(..) method.
> 
> > Tools/Scripts/webkitpy/layout_tests/reftests/lru_unittest.py:28
> > +import lru
> 
> We should use absolute path in import.
> like:
>   from webkipy.common.xxx import lru

-- 
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