add SkChecksum as a static class, for the replacement API

add SkChecksum as a static class, for the replacement API
after this lands, plan to deprecate/remove the older APIs
Committed: https://code.google.com/p/skia/source/detail?r=4457

[reed1, 2012-07-03 17:57:38]

Here are a 32bit and 64bit run of the bench (both compiled on a Mac)

32bit:

running bench [640 480]          compute_checksum_XX  8888: cmsecs =  22.28  
565: cmsecs =  22.27   GPU: cmsecs =  22.28  NULLGPU: cmsecs =  22.28
running bench [640 480]          compute_checksum_64  8888: cmsecs =  28.40  
565: cmsecs =  28.40   GPU: cmsecs =  28.40  NULLGPU: cmsecs =  28.42
running bench [640 480]          compute_checksum_32  8888: cmsecs =  24.48  
565: cmsecs =  24.47   GPU: cmsecs =  24.48  NULLGPU: cmsecs =  24.48

64bit:

running bench [640 480]          compute_checksum_XX  8888: cmsecs =  10.78  
565: cmsecs =  10.78   GPU: cmsecs =  10.78  NULLGPU: cmsecs =  10.78
running bench [640 480]          compute_checksum_64  8888: cmsecs =  11.95  
565: cmsecs =  11.95   GPU: cmsecs =  11.95  NULLGPU: cmsecs =  11.95
running bench [640 480]          compute_checksum_32  8888: cmsecs =  24.58  
565: cmsecs =  24.58   GPU: cmsecs =  24.58  NULLGPU: cmsecs =  24.58

[junov1, 2012-07-03 18:47:27]

https://codereview.appspot.com/6356059/diff/1/include/core/SkChecksum.h
File include/core/SkChecksum.h (right):

https://codereview.appspot.com/6356059/diff/1/include/core/SkChecksum.h#newco...
include/core/SkChecksum.h:79: static inline uintptr_t Mash(uintptr_t total,
uintptr_t value) {
As discussed earlier, this might not inline with Visual Studio, which is why it
was previously written as a macro.  Alternately, you could use __forceinline in
VC++ builds

https://codereview.appspot.com/6356059/diff/1/include/core/SkChecksum.h#newco...
include/core/SkChecksum.h:96: const uintptr_t* ptr = reinterpret_cast<const
uintptr_t*>(data);
32/64 switch... Nice and subtle :-)

https://codereview.appspot.com/6356059/diff/1/include/core/SkChecksum.h#newco...
include/core/SkChecksum.h:116: result ^= result >> HALFBITS;
You need to AND with 0xFFFFFFFFULL otherwise the static_cast below will clamp to
0xFFFFFFFF, making the checksum quite useless.

[bsalomon, 2012-07-03 18:55:01]

https://codereview.appspot.com/6356059/diff/1/include/core/SkChecksum.h
File include/core/SkChecksum.h (right):

https://codereview.appspot.com/6356059/diff/1/include/core/SkChecksum.h#newco...
include/core/SkChecksum.h:116: result ^= result >> HALFBITS;
On 2012/07/03 18:47:28, junov1 wrote:
> You need to AND with 0xFFFFFFFFULL otherwise the static_cast below will clamp
to
> 0xFFFFFFFF, making the checksum quite useless.

I'm pretty sure it just takes the low bits without any clamping.

[junov1, 2012-07-03 19:06:47]

You're, right.  Just tested it.

[reed1, 2012-07-03 19:12:36]

The Mash function is getting inlined (checked the disassembly). Skia has gobs of
those sorts of trivial functions written as functions and not macros. I think VS
has a problem with larger functions that have branches/loops.

[reed1, 2012-07-03 19:21:59]

patch #2 has updated comments, to make some of the tricky 32/64 magic clearer.

I may land this now, but not convert any of our call-sites to use it (other than
the bench) until it has baked a while.

[bsalomon, 2012-07-03 19:28:58]

https://codereview.appspot.com/6356059/diff/6001/include/core/SkChecksum.h
File include/core/SkChecksum.h (right):

https://codereview.appspot.com/6356059/diff/6001/include/core/SkChecksum.h#ne...
include/core/SkChecksum.h:71: class SkChecksum : SkNoncopyable {
namespace?

[reed1, 2012-07-03 19:42:16]

On 2012/07/03 19:28:58, bsalomon wrote:
> https://codereview.appspot.com/6356059/diff/6001/include/core/SkChecksum.h
> File include/core/SkChecksum.h (right):
> 
>
https://codereview.appspot.com/6356059/diff/6001/include/core/SkChecksum.h#ne...
> include/core/SkChecksum.h:71: class SkChecksum : SkNoncopyable {
> namespace?

namespace can also work, but so far we don't have a formal namespace that has
leaked into our public APIs. We use static classes like this one for that
purpose : SkFontHost, SkGradientShader, etc. Something to consider for the
future perhaps.

[bsalomon, 2012-07-03 19:53:53]

On 2012/07/03 19:42:16, reed1 wrote:
> On 2012/07/03 19:28:58, bsalomon wrote:
> > https://codereview.appspot.com/6356059/diff/6001/include/core/SkChecksum.h
> > File include/core/SkChecksum.h (right):
> > 
> >
>
https://codereview.appspot.com/6356059/diff/6001/include/core/SkChecksum.h#ne...
> > include/core/SkChecksum.h:71: class SkChecksum : SkNoncopyable {
> > namespace?
> 
> namespace can also work, but so far we don't have a formal namespace that has
> leaked into our public APIs. We use static classes like this one for that
> purpose : SkFontHost, SkGradientShader, etc. Something to consider for the
> future perhaps.

LGTM

[junov1, 2012-07-04 13:56:17]

On 2012/07/03 19:53:53, bsalomon wrote:
> On 2012/07/03 19:42:16, reed1 wrote:
> > On 2012/07/03 19:28:58, bsalomon wrote:
> > > https://codereview.appspot.com/6356059/diff/6001/include/core/SkChecksum.h
> > > File include/core/SkChecksum.h (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/6356059/diff/6001/include/core/SkChecksum.h#ne...
> > > include/core/SkChecksum.h:71: class SkChecksum : SkNoncopyable {
> > > namespace?
> > 
> > namespace can also work, but so far we don't have a formal namespace that
has
> > leaked into our public APIs. We use static classes like this one for that
> > purpose : SkFontHost, SkGradientShader, etc. Something to consider for the
> > future perhaps.
> 
> LGTM

lgtm