Add an SkSet class. At the moment we need to preserve the order of element in SkSet to have minimal…

Add a true Set class. Current use case is to be used with Pdf generator.
Committed: https://code.google.com/p/skia/source/detail?r=7626

[reed1, 2012-10-22 12:22:15]

For now, since its new, and since its crazy (not sorted), lets move the file
into src/pdf/SkTPDFSet.h or some name like that (the 'T' is for template).

More comments soon.

[edisonn, 2012-10-22 16:39:37]

On 2012/10/22 12:22:15, reed1 wrote:
> For now, since its new, and since its crazy (not sorted), lets move the file
> into src/pdf/SkTPDFSet.h or some name like that (the 'T' is for template).
> 
> More comments soon.

moved to include/pdf/SkTPDFSet.h as it is needed in some files in that directory

[edisonn, 2012-10-31 15:12:08]

Ping

[Steve VanDeBogart, 2012-10-31 17:44:31]

https://codereview.appspot.com/6749054/diff/3002/include/pdf/SkTPDFSet.h
File include/pdf/SkTPDFSet.h (right):

https://codereview.appspot.com/6749054/diff/3002/include/pdf/SkTPDFSet.h#newc...
include/pdf/SkTPDFSet.h:35: elem->ref();
I think this means that this can only ever be used when T has a ref() method.

It also means that ownership for the inserted items is ambiguous - does this
class own a reference or not?

You could have this template take a traits class and have two traits (to start):
NonOwnedTraits and RefCountedOwnedTraits.  Then you could remove the
insertAndRef method and just have an insert method.  With the
RefCountedOwnedTraits it would ref on insert and unref on remove/destruction.

https://codereview.appspot.com/6749054/diff/3002/include/pdf/SkTPDFSet.h#newc...
include/pdf/SkTPDFSet.h:63: void rewind() {
Omit this method, it's the same as reset(), right?

https://codereview.appspot.com/6749054/diff/3002/include/pdf/SkTPDFSet.h#newc...
include/pdf/SkTPDFSet.h:93: void unrefAll() {
These two could become private/go away with the traits.

[reed1, 2012-10-31 17:49:30]

lgtm

https://codereview.appspot.com/6749054/diff/3002/include/pdf/SkTPDFSet.h
File include/pdf/SkTPDFSet.h (right):

https://codereview.appspot.com/6749054/diff/3002/include/pdf/SkTPDFSet.h#newc...
include/pdf/SkTPDFSet.h:35: elem->ref();
On 2012/10/31 17:44:31, Steve VanDeBogart wrote:
> I think this means that this can only ever be used when T has a ref() method.
> 
> It also means that ownership for the inserted items is ambiguous - does this
> class own a reference or not?
> 
> You could have this template take a traits class and have two traits (to
start):
> NonOwnedTraits and RefCountedOwnedTraits.  Then you could remove the
> insertAndRef method and just have an insert method.  With the
> RefCountedOwnedTraits it would ref on insert and unref on remove/destruction.

Though that would force the decision on the caller that its ref-class would
*always* be owned by the set, instead of letting it be a runtime decision as it
is here. I think having two insert methods is OK.

https://codereview.appspot.com/6749054/diff/3002/include/pdf/SkTPDFSet.h#newc...
include/pdf/SkTPDFSet.h:63: void rewind() {
On 2012/10/31 17:44:31, Steve VanDeBogart wrote:
> Omit this method, it's the same as reset(), right?

Rewind keeps the allocation, but zeros the counters. reset() frees the
allocation.

[edisonn, 2012-11-02 20:11:13]

Updated the pdf code to use a true Set class. Please review this code.

[reed1, 2012-11-02 20:15:12]

general comments for a new utility class.

1. store it in src/core or src/pdf for now
2. write unittests
3. consider writing bench(s) for the new class
4. add more dox to the header

[edisonn, 2012-11-05 13:49:23]

On 2012/11/02 20:15:12, reed1 wrote:
> general comments for a new utility class.
> 
> 1. store it in src/core or src/pdf for now
done
> 2. write unittests
done
> 3. consider writing bench(s) for the new class
added todo, I will add later,   before/when will be moved to public
> 4. add more dox to the header
done

[reed1, 2012-11-05 16:04:05]

lgtm

[Steve VanDeBogart, 2012-11-05 22:35:57]

The interface looks cleaner and I have no objections but I haven't reviewed the
code. LGTM