Issue 6869049: Add SkTLList, linked list class implemented on top of the internal llist class.

Issue 6869049: Add SkTLList, linked list class implemented on top of the internal llist class. (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
11 years, 6 months ago by bsalomon

Modified:
11 years, 6 months ago

Reviewers:
robertphillips

CC:
skia-review_googlegroups.com

Base URL:
http://skia.googlecode.com/svn/trunk/

Visibility:
Public.

More Reviews

Description

Add SkTLList, linked list class implemented on top of the internal llist class. R=robertphillips@google.com Committed: https://code.google.com/p/skia/source/detail?r=6644 Committed: https://code.google.com/p/skia/source/detail?r=6647

Patch Set 1 #

Total comments: 24

Patch Set 2 : Add SkTLList, linked list class implemented on top of the internal llist class. #

Created: 11 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+430 lines, -104 lines)			Patch
M	gyp/core.gypi	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	gyp/tests.gyp	View	1	2 chunks	+1 line, -1 line	0 comments	Download
A	src/core/SkTLList.h	View	1	1 chunk	+272 lines, -0 lines	0 comments	Download
A +	tests/LListTest.cpp	View	1	4 chunks	+156 lines, -13 lines	0 comments	Download
D	tests/TDLinkedListTest.cpp	View	1	1 chunk	+0 lines, -90 lines	0 comments	Download

Messages

Total messages: 4

Expand All Messages | Collapse All Messages

robertphillips

LGTM + suggestions/questions https://codereview.appspot.com/6869049/diff/1/src/core/SkTLList.h File src/core/SkTLList.h (right): https://codereview.appspot.com/6869049/diff/1/src/core/SkTLList.h#newcode12 src/core/SkTLList.h:12: the list creates the objects and ...

11 years, 6 months ago (2012-12-03 16:04:48 UTC) #2

bsalomon

Add SkTLList, linked list class implemented on top of the internal llist class. R=robertphillips@google.com

11 years, 6 months ago (2012-12-03 16:17:55 UTC) #3

bsalomon

11 years, 6 months ago (2012-12-03 18:03:11 UTC) #4

Message was sent while issue was closed.

https://codereview.appspot.com/6869049/diff/1/src/core/SkTLList.h
File src/core/SkTLList.h (right):

https://codereview.appspot.com/6869049/diff/1/src/core/SkTLList.h#newcode12
src/core/SkTLList.h:12: the list creates the objects and they are deleted upon
removal. */
On 2012/12/03 16:04:48, robertphillips wrote:
> // This class in intended to add block allocation and linked list
functionality
> to an arbitrary class

Done.

https://codereview.appspot.com/6869049/diff/1/src/core/SkTLList.h#newcode19
src/core/SkTLList.h:19: SK_DECLARE_INTERNAL_LLIST_INTERFACE(Node);
On 2012/12/03 16:04:48, robertphillips wrote:
> // back pointer to owning block

Done.

https://codereview.appspot.com/6869049/diff/1/src/core/SkTLList.h#newcode40
src/core/SkTLList.h:40: if (0 == --block->fNodesInUse) {
On 2012/12/03 16:04:48, robertphillips wrote:
> Move the sk_free after the ~Node calls?

Done.

https://codereview.appspot.com/6869049/diff/1/src/core/SkTLList.h#newcode104
src/core/SkTLList.h:104: int count() const { return fCount; }
On 2012/12/03 16:04:48, robertphillips wrote:
> 0 == fCount instead?

Done.

https://codereview.appspot.com/6869049/diff/1/src/core/SkTLList.h#newcode116
src/core/SkTLList.h:116: a.next(), b.next()) {
On 2012/12/03 16:04:48, robertphillips wrote:
> Turn this into: "if (NULL == b.get()) { return false; }"?

b should never be NULL because we first check if the counts match and we already
checked that a is not NULL. Added comment.

https://codereview.appspot.com/6869049/diff/1/src/core/SkTLList.h#newcode128
src/core/SkTLList.h:128: private:
On 2012/12/03 16:04:48, robertphillips wrote:
> Can this not go with the other private stuff at the bottom?
I'm using INHERITED::IterStart before private

https://codereview.appspot.com/6869049/diff/1/src/core/SkTLList.h#newcode137
src/core/SkTLList.h:137: 
On 2012/12/03 16:04:48, robertphillips wrote:
> I thought we weren't calling INHERITED()?

Done.

https://codereview.appspot.com/6869049/diff/1/src/core/SkTLList.h#newcode174
src/core/SkTLList.h:174: Node* createNode() {
On 2012/12/03 16:04:48, robertphillips wrote:
> Node* node = fFreeList.head();
> if (NULL != node) {
> ?

Done.

https://codereview.appspot.com/6869049/diff/1/src/core/SkTLList.h#newcode199
src/core/SkTLList.h:199: if (0 == --node->fBlock->fNodesInUse) {
On 2012/12/03 16:04:48, robertphillips wrote:
> // Opportunistically delete unused blocks to reduce memory consumption

Done.

https://codereview.appspot.com/6869049/diff/1/tests/LListTest.cpp
File tests/LListTest.cpp (right):

https://codereview.appspot.com/6869049/diff/1/tests/LListTest.cpp#newcode126
tests/LListTest.cpp:126: 
On 2012/12/03 16:04:48, robertphillips wrote:
> "to the tail"

Done.

https://codereview.appspot.com/6869049/diff/1/tests/LListTest.cpp#newcode156
tests/LListTest.cpp:156: #endif
On 2012/12/03 16:04:48, robertphillips wrote:
> Maybe "REPORTER_ASSERT(reporter, !list1.isEmpty() && !list2.isEmpty());" here?

Done.

https://codereview.appspot.com/6869049/diff/1/tests/LListTest.cpp#newcode174
tests/LListTest.cpp:174: } else {
On 2012/12/03 16:04:48, robertphillips wrote:
> Do we want to checkin this brkpt code?

Done.

Expand All Messages | Collapse All Messages