Create SkBitSet class for font subsetting

Create SkBitSet class for font subsetting

Separate change from CR 4633050


BUG=none
TEST=none

[arthurhsu, 2011-06-29 22:43:00]

Have problems with git for previous attempt, please use this one instead,
thanks.

[arthurhsu, 2011-06-29 22:45:54]

Sorry for deleting previous issue.  I didn't realize I can use git cl <issue>.

[Steve VanDeBogart, 2011-06-29 22:55:00]

http://codereview.appspot.com/4627077/diff/1/include/pdf/SkBitSet.h
File include/pdf/SkBitSet.h (right):

http://codereview.appspot.com/4627077/diff/1/include/pdf/SkBitSet.h#newcode24
include/pdf/SkBitSet.h:24: SkBitSet();
Without reset, I don't think the empty constructor is useful anymore.

http://codereview.appspot.com/4627077/diff/1/include/pdf/SkBitSet.h#newcode43
include/pdf/SkBitSet.h:43: void clone(const SkBitSet& source);
Drop this.

http://codereview.appspot.com/4627077/diff/1/src/pdf/SkBitSet.cpp
File src/pdf/SkBitSet.cpp (right):

http://codereview.appspot.com/4627077/diff/1/src/pdf/SkBitSet.cpp#newcode24
src/pdf/SkBitSet.cpp:24: resize(numberOfBits);
This is the only caller to resize, inline and simply it.

http://codereview.appspot.com/4627077/diff/1/src/pdf/SkBitSet.cpp#newcode31
src/pdf/SkBitSet.cpp:31: SkBitSet::~SkBitSet() {
This isn't needed - remove it.

http://codereview.appspot.com/4627077/diff/1/src/pdf/SkBitSet.cpp#newcode36
src/pdf/SkBitSet.cpp:36: reset();
After removing the destructor, this is the only caller of reset(), inline it.

http://codereview.appspot.com/4627077/diff/1/src/pdf/SkBitSet.cpp#newcode64
src/pdf/SkBitSet.cpp:64: memset(newBitmap, 0, newSize * sizeof(int32_t));
call clearAll()

http://codereview.appspot.com/4627077/diff/1/src/pdf/SkBitSet.cpp#newcode91
src/pdf/SkBitSet.cpp:91: mask = 1 << (index & 0x1f);  // modulo 32
Let the compiler do the optimization: index % 32;

http://codereview.appspot.com/4627077/diff/1/src/pdf/SkBitSet.cpp#newcode94
src/pdf/SkBitSet.cpp:94: mask = ~(1 << (index & 0x1f));  // modulo 32
Pull mask outside the if, and just do &= ~mask

http://codereview.appspot.com/4627077/diff/1/src/pdf/SkBitSet.cpp#newcode100
src/pdf/SkBitSet.cpp:100: uint32_t mask = 1 << (index & 0x1f);  // modulo 32
Let the compiler do the optimization: index % 32;

http://codereview.appspot.com/4627077/diff/1/src/pdf/SkBitSet.cpp#newcode106
src/pdf/SkBitSet.cpp:106: *(internalGet(i)) |= *(source.internalGet(i));
This has a bunch of unnecessary overhead.  start with a = internalGet(0) and
then loop over i *a[i] != ...

http://codereview.appspot.com/4627077/diff/1/src/pdf/SkBitSet.cpp#newcode112
src/pdf/SkBitSet.cpp:112: size_t internalIndex = index >> 5;  // divided by 32
Let the compiler do the optimization: index / 32;

[arthurhsu, 2011-06-30 00:12:46]

http://codereview.appspot.com/4627077/diff/1/include/pdf/SkBitSet.h
File include/pdf/SkBitSet.h (right):

http://codereview.appspot.com/4627077/diff/1/include/pdf/SkBitSet.h#newcode24
include/pdf/SkBitSet.h:24: SkBitSet();
On 2011/06/29 22:55:00, Steve VanDeBogart wrote:
> Without reset, I don't think the empty constructor is useful anymore.

Done.

http://codereview.appspot.com/4627077/diff/1/include/pdf/SkBitSet.h#newcode43
include/pdf/SkBitSet.h:43: void clone(const SkBitSet& source);
On 2011/06/29 22:55:00, Steve VanDeBogart wrote:
> Drop this.

Done.

http://codereview.appspot.com/4627077/diff/1/src/pdf/SkBitSet.cpp
File src/pdf/SkBitSet.cpp (right):

http://codereview.appspot.com/4627077/diff/1/src/pdf/SkBitSet.cpp#newcode24
src/pdf/SkBitSet.cpp:24: resize(numberOfBits);
On 2011/06/29 22:55:00, Steve VanDeBogart wrote:
> This is the only caller to resize, inline and simply it.

Done.

http://codereview.appspot.com/4627077/diff/1/src/pdf/SkBitSet.cpp#newcode31
src/pdf/SkBitSet.cpp:31: SkBitSet::~SkBitSet() {
On 2011/06/29 22:55:00, Steve VanDeBogart wrote:
> This isn't needed - remove it.

Done.

http://codereview.appspot.com/4627077/diff/1/src/pdf/SkBitSet.cpp#newcode36
src/pdf/SkBitSet.cpp:36: reset();
On 2011/06/29 22:55:00, Steve VanDeBogart wrote:
> After removing the destructor, this is the only caller of reset(), inline it.

Done.

http://codereview.appspot.com/4627077/diff/1/src/pdf/SkBitSet.cpp#newcode64
src/pdf/SkBitSet.cpp:64: memset(newBitmap, 0, newSize * sizeof(int32_t));
On 2011/06/29 22:55:00, Steve VanDeBogart wrote:
> call clearAll()

Done.

http://codereview.appspot.com/4627077/diff/1/src/pdf/SkBitSet.cpp#newcode91
src/pdf/SkBitSet.cpp:91: mask = 1 << (index & 0x1f);  // modulo 32
On 2011/06/29 22:55:00, Steve VanDeBogart wrote:
> Let the compiler do the optimization: index % 32;

Done.

http://codereview.appspot.com/4627077/diff/1/src/pdf/SkBitSet.cpp#newcode94
src/pdf/SkBitSet.cpp:94: mask = ~(1 << (index & 0x1f));  // modulo 32
On 2011/06/29 22:55:00, Steve VanDeBogart wrote:
> Pull mask outside the if, and just do &= ~mask

Done.

http://codereview.appspot.com/4627077/diff/1/src/pdf/SkBitSet.cpp#newcode100
src/pdf/SkBitSet.cpp:100: uint32_t mask = 1 << (index & 0x1f);  // modulo 32
On 2011/06/29 22:55:00, Steve VanDeBogart wrote:
> Let the compiler do the optimization: index % 32;

Done.

http://codereview.appspot.com/4627077/diff/1/src/pdf/SkBitSet.cpp#newcode106
src/pdf/SkBitSet.cpp:106: *(internalGet(i)) |= *(source.internalGet(i));
On 2011/06/29 22:55:00, Steve VanDeBogart wrote:
> This has a bunch of unnecessary overhead.  start with a = internalGet(0) and
> then loop over i *a[i] != ...

Done.

http://codereview.appspot.com/4627077/diff/1/src/pdf/SkBitSet.cpp#newcode112
src/pdf/SkBitSet.cpp:112: size_t internalIndex = index >> 5;  // divided by 32
On 2011/06/29 22:55:00, Steve VanDeBogart wrote:
> Let the compiler do the optimization: index / 32;

Done.

[reed1, 2011-06-30 12:58:55]

I like the cleaner api.

I would consider making internalGet() inlined, since in the release version its
impl will probably be smaller than its function call overhead.

I'm sorry we loss storing the actual bit-count, since now we can't assert that
the caller is out-of-range when they call us.

[Steve VanDeBogart, 2011-06-30 16:16:45]

On 2011/06/30 12:58:55, reed1 wrote:
> I like the cleaner api.
> 
> I would consider making internalGet() inlined, since in the release version
its
> impl will probably be smaller than its function call overhead.
> 
> I'm sorry we loss storing the actual bit-count, since now we can't assert that
> the caller is out-of-range when they call us.

There's a comment on the constructor noting that the storage size is rounded up.
 The only place where it might cause a problem is when the code constructing the
bitset and the code using it don't agree on the size.  But there's no mechanism
to get the size, so it'll already be required that they communicate that OOB.

Arthur: consider inlining internalGet and my nits and then I'll commit this.

http://codereview.appspot.com/4627077/diff/7001/include/pdf/SkBitSet.h
File include/pdf/SkBitSet.h (right):

http://codereview.appspot.com/4627077/diff/7001/include/pdf/SkBitSet.h#newcode24
include/pdf/SkBitSet.h:24: /** The actual size will be rounded to 32-bit
boundary.
nit: "...rounded up to a..."

http://codereview.appspot.com/4627077/diff/7001/include/pdf/SkBitSet.h#newcode51
include/pdf/SkBitSet.h:51: SkAutoFree fBitmap;
nit: fBitmap is a confusing name considering this is a graphics library.  How
about fBitValues or fBitData?

http://codereview.appspot.com/4627077/diff/7001/include/pdf/SkBitSet.h#newcode53
include/pdf/SkBitSet.h:53: size_t fDwordCount;
nit: fCount is probably sufficiently specific.

http://codereview.appspot.com/4627077/diff/7001/src/pdf/SkBitSet.cpp
File src/pdf/SkBitSet.cpp (right):

http://codereview.appspot.com/4627077/diff/7001/src/pdf/SkBitSet.cpp#newcode23
src/pdf/SkBitSet.cpp:23: size_t newSize = 0;
nit: newSize isn't needed, you can use fDwordCount directly.

http://codereview.appspot.com/4627077/diff/7001/src/pdf/SkBitSet.cpp#newcode58
src/pdf/SkBitSet.cpp:58: if (fBitmap.get() != NULL) {
nit: SkASSERT?  I don't see anyway for fBitmap to be NULL now.

http://codereview.appspot.com/4627077/diff/7001/src/pdf/SkBitSet.cpp#newcode66
src/pdf/SkBitSet.cpp:66: *(internalGet(index)) |= mask;  // set
nit: Comments seem unnecessary.

[reed1, 2011-06-30 16:31:33]

On 2011/06/30 16:16:45, Steve VanDeBogart wrote:
> On 2011/06/30 12:58:55, reed1 wrote:
> > I like the cleaner api.
> > 
> > I would consider making internalGet() inlined, since in the release version
> its
> > impl will probably be smaller than its function call overhead.
> > 
> > I'm sorry we loss storing the actual bit-count, since now we can't assert
that
> > the caller is out-of-range when they call us.
> 
> There's a comment on the constructor noting that the storage size is rounded
up.
>  The only place where it might cause a problem is when the code constructing
the
> bitset and the code using it don't agree on the size.  But there's no
mechanism
> to get the size, so it'll already be required that they communicate that OOB.

Not sure I follow. I'm suggesting something like

bool isBitSet(int index) {
    SkASSERT(index < fBitCount);
    ...
}

I don't see how to do that without store the actual bit-count somewhere. I'd be
OK if fBitCount was debug-only.

    SkDEBUGCODE(int fBitCount;)

> 
> Arthur: consider inlining internalGet and my nits and then I'll commit this.
> 
> http://codereview.appspot.com/4627077/diff/7001/include/pdf/SkBitSet.h
> File include/pdf/SkBitSet.h (right):
> 
>
http://codereview.appspot.com/4627077/diff/7001/include/pdf/SkBitSet.h#newcode24
> include/pdf/SkBitSet.h:24: /** The actual size will be rounded to 32-bit
> boundary.
> nit: "...rounded up to a..."
> 
>
http://codereview.appspot.com/4627077/diff/7001/include/pdf/SkBitSet.h#newcode51
> include/pdf/SkBitSet.h:51: SkAutoFree fBitmap;
> nit: fBitmap is a confusing name considering this is a graphics library.  How
> about fBitValues or fBitData?
> 
>
http://codereview.appspot.com/4627077/diff/7001/include/pdf/SkBitSet.h#newcode53
> include/pdf/SkBitSet.h:53: size_t fDwordCount;
> nit: fCount is probably sufficiently specific.

Actually, I like the big name, so I don't ever thing fCount means bit-count
(since it doesn't).

> 
> http://codereview.appspot.com/4627077/diff/7001/src/pdf/SkBitSet.cpp
> File src/pdf/SkBitSet.cpp (right):
> 
> http://codereview.appspot.com/4627077/diff/7001/src/pdf/SkBitSet.cpp#newcode23
> src/pdf/SkBitSet.cpp:23: size_t newSize = 0;
> nit: newSize isn't needed, you can use fDwordCount directly.
> 
> http://codereview.appspot.com/4627077/diff/7001/src/pdf/SkBitSet.cpp#newcode58
> src/pdf/SkBitSet.cpp:58: if (fBitmap.get() != NULL) {
> nit: SkASSERT?  I don't see anyway for fBitmap to be NULL now.
> 
> http://codereview.appspot.com/4627077/diff/7001/src/pdf/SkBitSet.cpp#newcode66
> src/pdf/SkBitSet.cpp:66: *(internalGet(index)) |= mask;  // set
> nit: Comments seem unnecessary.

[arthurhsu, 2011-06-30 18:04:57]

http://codereview.appspot.com/4627077/diff/7001/include/pdf/SkBitSet.h
File include/pdf/SkBitSet.h (right):

http://codereview.appspot.com/4627077/diff/7001/include/pdf/SkBitSet.h#newcode24
include/pdf/SkBitSet.h:24: /** The actual size will be rounded to 32-bit
boundary.
On 2011/06/30 16:16:45, Steve VanDeBogart wrote:
> nit: "...rounded up to a..."

Done.

http://codereview.appspot.com/4627077/diff/7001/include/pdf/SkBitSet.h#newcode51
include/pdf/SkBitSet.h:51: SkAutoFree fBitmap;
On 2011/06/30 16:16:45, Steve VanDeBogart wrote:
> nit: fBitmap is a confusing name considering this is a graphics library.  How
> about fBitValues or fBitData?

Done.

http://codereview.appspot.com/4627077/diff/7001/include/pdf/SkBitSet.h#newcode53
include/pdf/SkBitSet.h:53: size_t fDwordCount;
On 2011/06/30 16:16:45, Steve VanDeBogart wrote:
> nit: fCount is probably sufficiently specific.

Dword tells the maintainer the exact unit used.

http://codereview.appspot.com/4627077/diff/7001/src/pdf/SkBitSet.cpp
File src/pdf/SkBitSet.cpp (right):

http://codereview.appspot.com/4627077/diff/7001/src/pdf/SkBitSet.cpp#newcode23
src/pdf/SkBitSet.cpp:23: size_t newSize = 0;
On 2011/06/30 16:16:45, Steve VanDeBogart wrote:
> nit: newSize isn't needed, you can use fDwordCount directly.

Done.

http://codereview.appspot.com/4627077/diff/7001/src/pdf/SkBitSet.cpp#newcode66
src/pdf/SkBitSet.cpp:66: *(internalGet(index)) |= mask;  // set
On 2011/06/30 16:16:45, Steve VanDeBogart wrote:
> nit: Comments seem unnecessary. 

Done.

[reed1, 2011-06-30 18:13:17]

Thanks for the debuggin fBitCount.

Now that I see it, and see operator==, I think it needs to be runtime, since two
bitsets will compare equal even if they have different bit-counts (potentially),
and they may compare != incorrectly if one of them went through the loop-or
function, where the parameter had more bits than the dst, and some of them were
set.

http://codereview.appspot.com/4627077/diff/11001/src/pdf/SkBitSet.cpp
File src/pdf/SkBitSet.cpp (right):

http://codereview.appspot.com/4627077/diff/11001/src/pdf/SkBitSet.cpp#newcode35
src/pdf/SkBitSet.cpp:35: fDwordCount = rhs.fDwordCount;
SkDEBUGCODE(fBitCount = rhs.fBitCount;)

http://codereview.appspot.com/4627077/diff/11001/src/pdf/SkBitSet.cpp#newcode42
src/pdf/SkBitSet.cpp:42: if (fDwordCount == rhs.fDwordCount) {
don't think the fDwordCount != 0 check is important.

[arthurhsu, 2011-06-30 18:26:13]

http://codereview.appspot.com/4627077/diff/11001/src/pdf/SkBitSet.cpp
File src/pdf/SkBitSet.cpp (right):

http://codereview.appspot.com/4627077/diff/11001/src/pdf/SkBitSet.cpp#newcode35
src/pdf/SkBitSet.cpp:35: fDwordCount = rhs.fDwordCount;
On 2011/06/30 18:13:17, reed1 wrote:
> SkDEBUGCODE(fBitCount = rhs.fBitCount;)

Done.

http://codereview.appspot.com/4627077/diff/11001/src/pdf/SkBitSet.cpp#newcode42
src/pdf/SkBitSet.cpp:42: if (fDwordCount == rhs.fDwordCount) {
On 2011/06/30 18:13:17, reed1 wrote:
> don't think the fDwordCount != 0 check is important.

Safe guard for release.  In release mode, if dword count is 0, bit data is null.
 memcmp() does not like it.  I don't mean people should do things like this, but
it won't hurt to pay little insurance like this.

[reed1, 2011-06-30 18:30:44]

Ah, I didn't know the bits would be null. Perhaps that could be the
check, rather than count... Just a thought.

On Thu, Jun 30, 2011 at 2:26 PM,  <arthurhsu@chromium.org> wrote:
>
> http://codereview.appspot.com/4627077/diff/11001/src/pdf/SkBitSet.cpp
> File src/pdf/SkBitSet.cpp (right):
>
>
http://codereview.appspot.com/4627077/diff/11001/src/pdf/SkBitSet.cpp#newcode35
> src/pdf/SkBitSet.cpp:35: fDwordCount = rhs.fDwordCount;
> On 2011/06/30 18:13:17, reed1 wrote:
>>
>> SkDEBUGCODE(fBitCount = rhs.fBitCount;)
>
> Done.
>
>
http://codereview.appspot.com/4627077/diff/11001/src/pdf/SkBitSet.cpp#newcode42
> src/pdf/SkBitSet.cpp:42: if (fDwordCount == rhs.fDwordCount) {
> On 2011/06/30 18:13:17, reed1 wrote:
>>
>> don't think the fDwordCount != 0 check is important.
>
> Safe guard for release.  In release mode, if dword count is 0, bit data
> is null.  memcmp() does not like it.  I don't mean people should do
> things like this, but it won't hurt to pay little insurance like this.
>
> http://codereview.appspot.com/4627077/
>

[Steve VanDeBogart, 2011-07-01 02:26:28]

This looks pretty good.  Baring objection, I'll commit this tomorrow.

http://codereview.appspot.com/4627077/diff/9003/src/pdf/SkBitSet.cpp
File src/pdf/SkBitSet.cpp (right):

http://codereview.appspot.com/4627077/diff/9003/src/pdf/SkBitSet.cpp#newcode64
src/pdf/SkBitSet.cpp:64: SkASSERT((size_t)index < fBitCount);
nit: push this into internalGet

http://codereview.appspot.com/4627077/diff/9003/src/pdf/SkBitSet.cpp#newcode64
src/pdf/SkBitSet.cpp:64: SkASSERT((size_t)index < fBitCount);
This needs to be wrapped in SkDEBUGCODE

[arthurhsu, 2011-07-01 03:13:23]

http://codereview.appspot.com/4627077/diff/9003/src/pdf/SkBitSet.cpp
File src/pdf/SkBitSet.cpp (right):

http://codereview.appspot.com/4627077/diff/9003/src/pdf/SkBitSet.cpp#newcode64
src/pdf/SkBitSet.cpp:64: SkASSERT((size_t)index < fBitCount);
On 2011/07/01 02:26:29, Steve VanDeBogart wrote:
> nit: push this into internalGet

Done.

http://codereview.appspot.com/4627077/diff/9003/src/pdf/SkBitSet.cpp#newcode64
src/pdf/SkBitSet.cpp:64: SkASSERT((size_t)index < fBitCount);
On 2011/07/01 02:26:29, Steve VanDeBogart wrote:
> This needs to be wrapped in SkDEBUGCODE

SkASSERT has no effect in Release mode.

[reed1, 2011-07-01 12:37:50]

SkBitSet set0(1);
SkBitSet set1(2);

set0.clearAll();
set1.clearAll();
SkASSERT(set0 != set1);   <--- this will fail

Do you expect set0 to be equal to set1? I presume they are not, since they have
different bit counts. I think the current impl thinks they are equal.


SkBitSet set0(1);
SkBitSet set1(1);
SkBitSet set2(2);

set0.setBit(0, true);
set1.setBit(0, true);
set2.setBit(1, true);

set0.orBIts(set2);
SkASSERT(s0 == s1);  <---- this will fail

Do you expect orBits to have changed set0 at all. In the current impl, it does,
since the orBits operation blindly takes all 32bits from the other bitset.

[Steve VanDeBogart, 2011-07-01 16:15:06]

Arthur, can you also write a few basic tests to add to the 'tests' target?

[arthurhsu, 2011-07-01 17:05:29]

On 2011/07/01 12:37:50, reed1 wrote:
> SkBitSet set0(1);
> SkBitSet set1(2);
> 
> set0.clearAll();
> set1.clearAll();
> SkASSERT(set0 != set1);   <--- this will fail
> 
> Do you expect set0 to be equal to set1? I presume they are not, since they
have
> different bit counts. I think the current impl thinks they are equal.
> 
> 
> SkBitSet set0(1);
> SkBitSet set1(1);
> SkBitSet set2(2);
> 
> set0.setBit(0, true);
> set1.setBit(0, true);
> set2.setBit(1, true);
> 
> set0.orBIts(set2);
> SkASSERT(s0 == s1);  <---- this will fail
> 
> Do you expect orBits to have changed set0 at all. In the current impl, it
does,
> since the orBits operation blindly takes all 32bits from the other bitset.

I've written the test cases and uploaded.
If two sets have different dword count, they shall not be equal.
If two sets have same dword count, and the dwords are identical, they are equal
(even bit count is different).

[reed1, 2011-07-01 21:46:22]

Nice.

Lots of small comments, none of which are mandatory, except the self-assign
check in operator=.

http://codereview.appspot.com/4627077/diff/17002/include/pdf/SkBitSet.h
File include/pdf/SkBitSet.h (right):

http://codereview.appspot.com/4627077/diff/17002/include/pdf/SkBitSet.h#newco...
include/pdf/SkBitSet.h:60: SkASSERT(index >= 0);
btw -- the 2nd assert is all you need. It will treat negative indices as very
very large, and so the assert will fire.

http://codereview.appspot.com/4627077/diff/17002/include/pdf/SkBitSet.h#newco...
include/pdf/SkBitSet.h:61: SkASSERT((size_t)index < fBitCount);
I *think* you save one instruction if you use index >> 5 instead of the divide,
as the compiler has to handle negative values differently if it tries to use a
shift internally.

http://codereview.appspot.com/4627077/diff/17002/src/pdf/SkBitSet.cpp
File src/pdf/SkBitSet.cpp (right):

http://codereview.appspot.com/4627077/diff/17002/src/pdf/SkBitSet.cpp#newcode34
src/pdf/SkBitSet.cpp:34: const SkBitSet& SkBitSet::operator=(const SkBitSet&
rhs) {
if (this == (SkBitSet*)rhs) return *this;

Need a self check, otherwise we free our data before we can copy it!

http://codereview.appspot.com/4627077/diff/17002/src/pdf/SkBitSet.cpp#newcode59
src/pdf/SkBitSet.cpp:59: if (fBitData.get() != NULL) {
for your convenience

sk_bzero(fBitData.get(), fDwordCount * sizeof(uint32_t));

http://codereview.appspot.com/4627077/diff/17002/src/pdf/SkBitSet.cpp#newcode64
src/pdf/SkBitSet.cpp:64: void SkBitSet::setBit(int index, bool value) {
See earlier divide comment. We *might* save an instruction if we said (index &
31) instead.

http://codereview.appspot.com/4627077/diff/17002/src/pdf/SkBitSet.cpp#newcode81
src/pdf/SkBitSet.cpp:81: uint32_t* sourceBitmap = source.internalGet(0);
I think the SkMin32 is now obsolete, since the bitcounts agree, the dwordcounts
must agree

[reed1, 2011-07-01 21:47:00]

Great to see a unittest!

[arthurhsu, 2011-07-01 22:11:26]

http://codereview.appspot.com/4627077/diff/17002/include/pdf/SkBitSet.h
File include/pdf/SkBitSet.h (right):

http://codereview.appspot.com/4627077/diff/17002/include/pdf/SkBitSet.h#newco...
include/pdf/SkBitSet.h:60: SkASSERT(index >= 0);
On 2011/07/01 21:46:22, reed1 wrote:
> btw -- the 2nd assert is all you need. It will treat negative indices as very
> very large, and so the assert will fire.

Done.

http://codereview.appspot.com/4627077/diff/17002/include/pdf/SkBitSet.h#newco...
include/pdf/SkBitSet.h:61: SkASSERT((size_t)index < fBitCount);
On 2011/07/01 21:46:22, reed1 wrote:
> I *think* you save one instruction if you use index >> 5 instead of the
divide,
> as the compiler has to handle negative values differently if it tries to use a
> shift internally.

Steve and I have talks regarding this.  On some platform, divide is faster than
shift, so it's better to let compilers do the optimization for us.  Anyway this
is not inside a loop and not critical either.

http://codereview.appspot.com/4627077/diff/17002/src/pdf/SkBitSet.cpp
File src/pdf/SkBitSet.cpp (right):

http://codereview.appspot.com/4627077/diff/17002/src/pdf/SkBitSet.cpp#newcode34
src/pdf/SkBitSet.cpp:34: const SkBitSet& SkBitSet::operator=(const SkBitSet&
rhs) {
On 2011/07/01 21:46:22, reed1 wrote:
> if (this == (SkBitSet*)rhs) return *this;
> 
> Need a self check, otherwise we free our data before we can copy it!

Done.

http://codereview.appspot.com/4627077/diff/17002/src/pdf/SkBitSet.cpp#newcode59
src/pdf/SkBitSet.cpp:59: if (fBitData.get() != NULL) {
On 2011/07/01 21:46:22, reed1 wrote:
> for your convenience
> 
> sk_bzero(fBitData.get(), fDwordCount * sizeof(uint32_t));

Done.

http://codereview.appspot.com/4627077/diff/17002/src/pdf/SkBitSet.cpp#newcode64
src/pdf/SkBitSet.cpp:64: void SkBitSet::setBit(int index, bool value) {
On 2011/07/01 21:46:22, reed1 wrote:
> See earlier divide comment. We *might* save an instruction if we said (index &
> 31) instead.

Compiler should take care of it for different platforms.

http://codereview.appspot.com/4627077/diff/17002/src/pdf/SkBitSet.cpp#newcode81
src/pdf/SkBitSet.cpp:81: uint32_t* sourceBitmap = source.internalGet(0);
On 2011/07/01 21:46:22, reed1 wrote:
> I think the SkMin32 is now obsolete, since the bitcounts agree, the
dwordcounts
> must agree

Done.

[Steve VanDeBogart, 2011-07-02 01:27:42]

Committed (with a couple last nits)
http://code.google.com/p/skia/source/detail?r=1788