Add SkSTDArray.

Add SkSTDArray.

[bungeman-skia, 2013-04-25 22:41:30]

As if Skia needed another container, this introduces SkSTDArray, based on how
SkSTArray is implemented. The advantage of the 'D' is that no initialization
takes place, with the 'ST' version the new elements are always default
initialized.

https://codereview.chromium.org/14502003/diff/2001/include/core/SkTDArray.h
File include/core/SkTDArray.h (left):

https://codereview.chromium.org/14502003/diff/2001/include/core/SkTDArray.h#o...
include/core/SkTDArray.h:343: ArrayT* fData;
I removed fData here as it was somewhat of a pain to keep around, and all modern
debuggers allow you to see any pointer as an array (with a@n or a,n) anyway.

[bungeman-skia, 2013-04-26 15:58:36]

With Patch Set 5 this all works more like it used to, and more like I want it
to. But the code is slightly more squirrely.

https://codereview.chromium.org/14502003/diff/6001/include/core/SkTDArray.h
File include/core/SkTDArray.h (right):

https://codereview.chromium.org/14502003/diff/6001/include/core/SkTDArray.h#n...
include/core/SkTDArray.h:66: if (fArray == fPreAllocMemArray) {
If these are both NULL (count==0 and this is SkTDArray) then this will be true,
returning a non-NULL pointer to a 0 length object on the heap.

[reed1, 2013-04-26 16:06:52]

https://codereview.chromium.org/14502003/diff/2001/include/core/SkTDArray.h
File include/core/SkTDArray.h (right):

https://codereview.chromium.org/14502003/diff/2001/include/core/SkTDArray.h#n...
include/core/SkTDArray.h:64: if (fArray == fPreAllocMemArray) {
 && fCount > 0 ?

https://codereview.chromium.org/14502003/diff/2001/include/core/SkTDArray.h#n...
include/core/SkTDArray.h:365: template <size_t N, typename T>
dox: N is the number of elements or the number of bytes?

https://codereview.chromium.org/14502003/diff/2001/include/core/SkTDArray.h#n...
include/core/SkTDArray.h:377: SkSTDArray(const SkSTDArray& src) : INHERITED(src,
&fStorage) {
Do we need to support copying from another S version instead of just SkTDArray&
src?

[bungeman-skia, 2013-04-26 16:26:16]

https://codereview.chromium.org/14502003/diff/2001/include/core/SkTDArray.h
File include/core/SkTDArray.h (right):

https://codereview.chromium.org/14502003/diff/2001/include/core/SkTDArray.h#n...
include/core/SkTDArray.h:64: if (fArray == fPreAllocMemArray) {
On 2013/04/26 16:06:52, reed1 wrote:
>  && fCount > 0 ?

I did this equivalently in PS5 with '&& fArray != NULL'. I was thinking about
this from the perspective of "if array starts out NULL then it should finish
NULL". Testing against count seems to be stating "if the count is 0, just return
whatever, it shouldn't matter". In this case both should have the same results
since I'm trying to maintain "if count==0 then fArray==NULL". This is true in
PS5, but not here in PS2.

https://codereview.chromium.org/14502003/diff/2001/include/core/SkTDArray.h#n...
include/core/SkTDArray.h:365: template <size_t N, typename T>
On 2013/04/26 16:06:52, reed1 wrote:
> dox: N is the number of elements or the number of bytes?

The number of elements. Will doc.

https://codereview.chromium.org/14502003/diff/2001/include/core/SkTDArray.h#n...
include/core/SkTDArray.h:377: SkSTDArray(const SkSTDArray& src) : INHERITED(src,
&fStorage) {
On 2013/04/26 16:06:52, reed1 wrote:
> Do we need to support copying from another S version instead of just
SkTDArray&
> src?

I modeled this after SkTArray/SkSTArray, which is to say have the 'S' version
have matching constructors to the 'T' version plus and explicit copy from
INHERITED. I think all other cases can be handled just as well by the (src,
count) constructor.

https://codereview.chromium.org/14502003/diff/7002/include/core/SkTDArray.h
File include/core/SkTDArray.h (right):

https://codereview.chromium.org/14502003/diff/7002/include/core/SkTDArray.h#n...
include/core/SkTDArray.h:352: if (NULL == fArray && fCount + extra <=
fPreAllocMemArraySize) {
Note that the same effect can be had by placing

if (NULL == fArray) {
    fArray = fPreAllocMemArray;
    fReserve = fPreAllocMemArraySize;
}

outside this if. However, I'm trying to avoid the overhead of doing this extra
test on every growBy, instead doing this test on the relatively less executed
resize path. If fArray is NULL, then fReserve is 0.

[bungeman-skia, 2013-04-29 21:28:33]

So I've found a simpler method of doing what I want to do in the DirectWrite
implementation, so I may not need this anymore. On the other hand, it's a lot of
work not to use. I think this is right, but I've started to go cross eyed
looking at it.

https://codereview.chromium.org/14502003/diff/7004/include/core/SkTDArray.h
File include/core/SkTDArray.h (right):

https://codereview.chromium.org/14502003/diff/7004/include/core/SkTDArray.h#n...
include/core/SkTDArray.h:55: SkTSwap(fPreAllocMemArray,
other.fPreAllocMemArray);
Actually, I don't think this is right. We don't want one stack guy pointing at
some other stack guys innards. It looks like swap is just here to support other
calls in this class, maybe we can remove it.

https://codereview.chromium.org/14502003/diff/7004/include/core/SkTDArray.h#n...
include/core/SkTDArray.h:331: if (count) {
Note that at this point we have a valid object. As a result, this block could be
replaced by

this->append(count, src);

This change makes this code fairly easy to read, but makes the code path beat
around the bush. It causes hard to measure changes in vc++. It also changes
behavior somewhat as growBy will over allocate memory where this does (and did)
not.

[reed1, 2013-04-30 17:40:41]

seems fine, lets park until needed.

[bungeman-skia, 2015-04-22 22:20:28]

bungeman@google.com changed reviewers:
+ joshualitt@google.com

[bungeman-skia, 2015-04-22 22:20:29]

Joshua was complaining this didn't exist today, so I dug it out of retirement
for him to take a look at it. It was surprisingly easy to do a quick merge (git
is pretty amazing), and being able to run CQ on a two year old review based on a
no longer existing project url is pretty cool. In any event, Joshua, it's all
yours.

https://codereview.chromium.org/14502003/diff/61001/include/core/SkTDArray.h
File include/core/SkTDArray.h (right):

https://codereview.chromium.org/14502003/diff/61001/include/core/SkTDArray.h#...
include/core/SkTDArray.h:64: void swap(SkTDArray<T>& other) {
This swap is wrong for any STD versions, haven't thought about how to fix.

[reed1, 2015-04-22 23:02:12]

Since it seems to add complexity (and overhead) to all SkTDArrays, is there any
way to add it "template-wise" so that instances that do not take advantage of it
don't have to pay for the extra code/size?

Where did Josh want to use this?

https://codereview.chromium.org/14502003/diff/61001/include/core/SkTDArray.h
File include/core/SkTDArray.h (right):

https://codereview.chromium.org/14502003/diff/61001/include/core/SkTDArray.h#...
include/core/SkTDArray.h:389: size_t fReserve;
Why change fReserve and fCount from int to size_t? Clearly in some places we had
to add casts. Can these stay ints?

[joshualitt, 2015-04-22 23:21:51]

On 2015/04/22 23:02:12, reed1 wrote:
> Since it seems to add complexity (and overhead) to all SkTDArrays, is there
any
> way to add it "template-wise" so that instances that do not take advantage of
it
> don't have to pay for the extra code/size?
> 
> Where did Josh want to use this?
src/gpu/GrBatchAtlas.h:98 SkSTArray<kMinItems, int, true> fPlotsToUpdate, I can,
however, use SkAutoSTMalloc and just manually do the copying / moving about
myself.  If I have a chance tomorrow, maybe I can find a way to reduce the cost.
> 
> https://codereview.chromium.org/14502003/diff/61001/include/core/SkTDArray.h
> File include/core/SkTDArray.h (right):
> 
>
https://codereview.chromium.org/14502003/diff/61001/include/core/SkTDArray.h#...
> include/core/SkTDArray.h:389: size_t fReserve;
> Why change fReserve and fCount from int to size_t? Clearly in some places we
had
> to add casts. Can these stay ints?