[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 #7 from Ai Makabi <makabi at google.com>  2011-09-15 07:29:49 PST ---
I addressed your comment.

(In reply to comment #5)
> (From update of attachment 107322 [details])
> Thank you! The patch looks good!
> I think there are still some corner cases we must handle carefully.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=107322&action=review
> 
> > Tools/Scripts/webkitpy/common/lru_cache.py:39
> > +        size: a cache memory size
> 
> 'capacity' might be a better name than 'size' in this context.
> 
> > Tools/Scripts/webkitpy/common/lru_cache.py:42
> > +        assert size > 0, "You should set the LRUCache size to not less than zero."
> 
> You might want to include the actual value of |capacity| in the assertion message.
> like:
>   assert capacity > 0, "capacity (%s) must be greater than zero." % capacity
> 
> > Tools/Scripts/webkitpy/common/lru_cache.py:43
> > +        self.first = None
> 
> if you don't want to expose |first| and |last|, that should be named like |_first| and |_last|.
> 
> > Tools/Scripts/webkitpy/common/lru_cache.py:49
> > +        if self.first == None:
> 
> I think 
>   if not self.first:
> is more pythonic than
>   if self.first == None:
> 
> At least it should be 'self.first is None' instead of 'self.first == None'.
> 
> > Tools/Scripts/webkitpy/common/lru_cache.py:65
> > +        node = Node(key, value, self.first)
> 
> When _one_node is called, self.first is always None, isn't it?
> Therefore this should be:
>   node = Node(key, value, None)
> 
> > Tools/Scripts/webkitpy/common/lru_cache.py:71
> > +        if self.first.key == key:
> 
> If self.first is None, this will fail.
> 
> > Tools/Scripts/webkitpy/common/lru_cache.py:99
> > +            self.first = node.prev
> 
> If LRUCache contains one item, (k, v) and del[k] is called, it seems self.first will become 'None' here. So the following self.first.next will fail. Could you add a testcase for this?
> 
> And if self.first and self.last point to the same node, it seems we also have to set self.last = None here.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.