[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