[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 #2 from Hayato Ito <hayato at chromium.org>  2011-09-14 01:33:58 PST ---
(From update of attachment 107297)
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.