Have peek32 return uint32_t& to make it harder to look at more than 4 bytes.

Have peek32 return uint32_t& to make it harder to look at more than 4 bytes.

BUG=skia:

Committed: http://code.google.com/p/skia/source/detail?r=13265

[reed1, 2014-01-31 14:26:08]

https://codereview.chromium.org/145053003/diff/1/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/145053003/diff/1/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:335: command = (command & MASK_24) | (NOOP << 24);
I like the idea, but I do find this line, and #371 much harder to read, since
we're modifying a "value" but it has side-effects.

Maybe its overkill, but could we....

Handle h = writer->getHandle32(offset);

value = writer->readHandle32(h);
writer->writeHandle32(h, value);
?

[reed1, 2014-01-31 14:26:35]

I don't feel strongly, so if others like this change, I'm fine too.

[mtklein, 2014-01-31 14:34:26]

https://codereview.chromium.org/145053003/diff/1/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/145053003/diff/1/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:335: command = (command & MASK_24) | (NOOP << 24);
Yeah, we could do that.  Most of these are just reads.  Maybe just this for now?
  uint32_t readOffset(size_t) const;
  void writeOffset(size_t, uint32_t);
Don't see a major advantage yet to having Handle != size_t.

On 2014/01/31 14:26:08, reed1 wrote:
> I like the idea, but I do find this line, and #371 much harder to read, since
> we're modifying a "value" but it has side-effects.
> 
> Maybe its overkill, but could we....
> 
> Handle h = writer->getHandle32(offset);
> 
> value = writer->readHandle32(h);
> writer->writeHandle32(h, value);
> ?

https://codereview.chromium.org/145053003/diff/1/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:370: uint32_t& paintId =
writer->peek32(dbmInfo.fOffset+dbmPaintOffset);
Clearer if I just write

write->peek32(dbmInfo.fOffset+dbmPaintOffset) = saveLayerPaintId

?

[robertphillips, 2014-01-31 14:57:37]

I'm in the same boat. Assigning back to the reference does seem a bit
side-effecty and seems like it could come back to bite us. But I do like the
idea of guarding the pointer access.

[reed1, 2014-01-31 15:03:25]

On 2014/01/31 14:57:37, robertphillips wrote:
> I'm in the same boat. Assigning back to the reference does seem a bit
> side-effecty and seems like it could come back to bite us. But I do like the
> idea of guarding the pointer access.

I suggested the extra cruft of a Handle in case we wanted to cache the work done
by lookup (if its not O(1)), since in at least one place, we did a
read/modify/write operation. If that concern is premature, then just read32At()
and write32At() are good for me.

[mtklein, 2014-01-31 15:37:03]

On 2014/01/31 15:03:25, reed1 wrote:
> On 2014/01/31 14:57:37, robertphillips wrote:
> > I'm in the same boat. Assigning back to the reference does seem a bit
> > side-effecty and seems like it could come back to bite us. But I do like the
> > idea of guarding the pointer access.
> 
Are you worried that dropping the *'s from the code makes it less clear we're
modifying the original?  Otherwise, I can only see a reference as strictly less
dangerous than a pointer.  As usual in C++, you can always shoot yourself in the
foot if you try hard enough, but you really have to go out of your way to, say,
read the next uint32_t when getting back a uint32_t&, where a uint32_t* makes it
super tempting.

> I suggested the extra cruft of a Handle in case we wanted to cache the work
done
> by lookup (if its not O(1)), since in at least one place, we did a
> read/modify/write operation. If that concern is premature, then just
read32At()
> and write32At() are good for me.

Let's worry about that if we get there.  It's O(1) now, and I've seen compilers
hoist that sort of calculation out of read/modify/write themselves (actually,
even when iterating via operator[] where you'd think you'd need a custom
iterator).

PTAL?

[reed1, 2014-01-31 15:47:39]

lgtm

[mtklein, 2014-01-31 15:57:45]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-01-31 15:57:57]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/mtklein@google.com/145053003/80001

[commit-bot: I haz the power, 2014-01-31 16:23:02]

Change committed as 13265

[commit-bot: I haz the power, 2014-01-31 16:23:05]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 16:23:06]

CQ bit was unchecked on CL. Ignoring.