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

src/core/SkPictureRecord.cpp

Index: src/core/SkPictureRecord.cpp
diff --git a/src/core/SkPictureRecord.cpp b/src/core/SkPictureRecord.cpp
index b2f9216df6a7b17b5bf1934af4ebfe93fda6b781..a4fa5d9b78b4a89b6a78a9831565d6ecef509cad 100644
--- a/src/core/SkPictureRecord.cpp
+++ b/src/core/SkPictureRecord.cpp
@@ -197,13 +197,13 @@ bool SkPictureRecord::isDrawingToLayer() const {
  * Read the op code from 'offset' in 'writer' and extract the size too.
  */
 static DrawType peek_op_and_size(SkWriter32* writer, int32_t offset, uint32_t* size) {
-    uint32_t* peek = writer->peek32(offset);
+    uint32_t peek = writer->peek32(offset);
 
     uint32_t op;
-    UNPACK_8_24(*peek, op, *size);
+    UNPACK_8_24(peek, op, *size);
     if (MASK_24 == *size) {
         // size required its own slot right after the op code
-        *size = *writer->peek32(offset+kUInt32Size);
+        *size = writer->peek32(offset+kUInt32Size);
     }
     return (DrawType) op;
 }
@@ -306,7 +306,7 @@ static bool remove_save_layer1(SkWriter32* writer, int32_t offset,
     // back up to the save block
     // TODO: add a stack to track save*/restore offsets rather than searching backwards
     while (offset > 0) {
-        offset = *writer->peek32(offset);
+        offset = writer->peek32(offset);
     }
 
     int pattern[] = { SAVE_LAYER, kDRAW_BITMAP_FLAVOR, /* RESTORE */ };
@@ -331,8 +331,8 @@ static bool remove_save_layer1(SkWriter32* writer, int32_t offset,
  * field alone so the NOOP can be skipped later.
  */
 static void convert_command_to_noop(SkWriter32* writer, uint32_t offset) {
-    uint32_t* ptr = writer->peek32(offset);
-    *ptr = (*ptr & MASK_24) | (NOOP << 24);
+    uint32_t& command = writer->peek32(offset);
+    command = (command & MASK_24) | (NOOP << 24);

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

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);
?

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

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:

 }
 
 /*
@@ -353,8 +353,8 @@ static bool merge_savelayer_paint_into_drawbitmp(SkWriter32* writer,
     uint32_t slPaintOffset = getPaintOffset(SAVE_LAYER, saveLayerInfo.fSize);
 
     // we have a match, now we need to get the paints involved
-    uint32_t dbmPaintId = *writer->peek32(dbmInfo.fOffset+dbmPaintOffset);
-    uint32_t saveLayerPaintId = *writer->peek32(saveLayerInfo.fOffset+slPaintOffset);
+    uint32_t dbmPaintId = writer->peek32(dbmInfo.fOffset+dbmPaintOffset);
+    uint32_t saveLayerPaintId = writer->peek32(saveLayerInfo.fOffset+slPaintOffset);
 
     if (0 == saveLayerPaintId) {
         // In this case the saveLayer/restore isn't needed at all - just kill the saveLayer
@@ -367,9 +367,8 @@ static bool merge_savelayer_paint_into_drawbitmp(SkWriter32* writer,
         // In this case just make the DBM* use the saveLayer's paint, kill the saveLayer
         // and signal the caller (by returning true) to not add the RESTORE op
         convert_command_to_noop(writer, saveLayerInfo.fOffset);
-        uint32_t* ptr = writer->peek32(dbmInfo.fOffset+dbmPaintOffset);
-        SkASSERT(0 == *ptr);
-        *ptr = saveLayerPaintId;
+        uint32_t& paintId = writer->peek32(dbmInfo.fOffset+dbmPaintOffset);

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

Clearer if I just write

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

?

+        paintId = saveLayerPaintId;
         return true;
     }
 
@@ -402,9 +401,8 @@ static bool merge_savelayer_paint_into_drawbitmp(SkWriter32* writer,
 
     // kill the saveLayer and alter the DBMR2R's paint to be the modified one
     convert_command_to_noop(writer, saveLayerInfo.fOffset);
-    uint32_t* ptr = writer->peek32(dbmInfo.fOffset+dbmPaintOffset);
-    SkASSERT(dbmPaintId == *ptr);
-    *ptr = data->index();
+    uint32_t& paintId = writer->peek32(dbmInfo.fOffset+dbmPaintOffset);
+    paintId = data->index();
     return true;
 }
 
@@ -425,7 +423,7 @@ static bool remove_save_layer2(SkWriter32* writer, int32_t offset,
     // back up to the save block
     // TODO: add a stack to track save*/restore offsets rather than searching backwards
     while (offset > 0) {
-        offset = *writer->peek32(offset);
+        offset = writer->peek32(offset);
     }
 
     int pattern[] = { SAVE_LAYER, SAVE, CLIP_RECT, kDRAW_BITMAP_FLAVOR, RESTORE, /* RESTORE */ };
@@ -462,7 +460,7 @@ static bool collapse_save_clip_restore(SkWriter32* writer, int32_t offset,
 
     // back up to the save block
     while (offset > 0) {
-        offset = *writer->peek32(offset);
+        offset = writer->peek32(offset);
     }
 
     // now offset points to a save
@@ -477,7 +475,7 @@ static bool collapse_save_clip_restore(SkWriter32* writer, int32_t offset,
     SkASSERT(kSaveSize == opSize);
 
     // get the save flag (last 4-bytes of the space allocated for the opSize)
-    SkCanvas::SaveFlags saveFlags = (SkCanvas::SaveFlags) *writer->peek32(offset+4);
+    SkCanvas::SaveFlags saveFlags = (SkCanvas::SaveFlags) writer->peek32(offset+4);
     if (SkCanvas::kMatrixClip_SaveFlag != saveFlags) {
         // This function's optimization is only correct for kMatrixClip style saves.
         // TODO: set checkMatrix & checkClip booleans here and then check for the
@@ -694,9 +692,9 @@ static bool regionOpExpands(SkRegion::Op op) {
 void SkPictureRecord::fillRestoreOffsetPlaceholdersForCurrentStackLevel(uint32_t restoreOffset) {
     int32_t offset = fRestoreOffsetStack.top();
     while (offset > 0) {
-        uint32_t* peek = fWriter.peek32(offset);
-        offset = *peek;
-        *peek = restoreOffset;
+        uint32_t& peek = fWriter.peek32(offset);
+        offset = peek;
+        peek = restoreOffset;
     }
 
 #ifdef SK_DEBUG