Templetized SkWriter32 readTAt() & overwriteTAt()

include/core/SkWriter32.h

Index: include/core/SkWriter32.h
diff --git a/include/core/SkWriter32.h b/include/core/SkWriter32.h
index b193cbe7c407663f7a82a613b8a65b5e1e637a47..902c2335fcd733a34f378dcfbb2de77e24e7d325 100644
--- a/include/core/SkWriter32.h
+++ b/include/core/SkWriter32.h
@@ -79,9 +79,23 @@ public:
         return p;
     }
 
-    // Read or write 4 bytes at offset, which must be a multiple of 4 <= size().
-    uint32_t read32At(size_t offset) { return this->atOffset(offset); }
-    void write32At(size_t offset, uint32_t val) { this->atOffset(offset) = val; }
+    // Read or (over)write a record at offset, which must be a multiple of 4.
+    // These calls are valid only if the same record type has been previously written
+    // atomically at the given offset.

[mtklein, 2014/02/06 21:38:16]

Now that we're looking at it again, it might make sense to rename writeTAt to
overwriteTAt.  It's different enough from the other write methods (it doesn't
reserve space) that it could use a very different name.

[mtklein, 2014/02/06 21:38:16]

This atomically isn't necessarily congruent to anything in the API.  Some
writeFoo calls are atomic, some are not.  Its hard for me to know whether say,
writeString is going to be contiguous.  

Maybe we should guarantee that every byte written within any single write call
is contiguous.  Then we replace "atomically" with "by a single call to
SkWriter32" and call it a day.  That's a nice guarantee to reason about, and
really, now that I'm looking, we'll only need to change writeString.

[f(malita), 2014/02/10 16:10:35]

Sounds good.

[f(malita), 2014/02/10 16:10:35]

Good catch - I hadn't realized some of the writeXYZ() calls are not atomic.
Sounds easy enough to fix, I'll take a look at writeString and update the
verbiage.

+    template<typename T>
+    inline const T& readTAt(size_t offset) const {
+        return this->atOffset<T>(offset);
+    }
+
+    template<typename T>
+    inline void writeTAt(size_t offset, T value) {
+        this->atOffset<T>(offset) = value;
+    }
+
+    template<typename T>
+    inline void writeT(const T& value) {

[mtklein, 2014/02/06 21:38:16]

My only reservation about doing this is that it allows us to write more types
publically to SkWriter32 than we used to.  That's more power I'd feel better
leaving inaccessible until we need it.

I'd feel a little cozier with a public API that looks more or less as it was,
maybe backed by private template code to make the implementation nicer.  I don't
mind overloading, so we can keep a public write() that handles everything we
support.  Here's one idea:

public:
  bool write(bool val) { return this->writeT<uint32_t>(val ? 1 : 0); }
  void write(uint8_t val) { this->writeT<uint32_t>(val); }
  void write(uint16_t val) { this->writeT<uint32_t>(val); }
  void write(uint32_t val) { this->writeT(val); }
  void write(SkScalar val) { this->writeT(val); }
  void write(const SkRect& val) { this->writeT(val); }
  template <typename T> void write(const T* val) { this->writeT(val); }
  ...
  // Special cases we may want to keep.
  void write8(uint32_t val) { this->writeT(val & 0xFF); }
  void write16(uint32_t val) { this->writeT(val & 0xFFFF); }

private:
  template <typename T>
  const T& writeT(const T& val) {
    SkASSERT(sizeof(T) % 4 == 0);
    *(T)this->reserve(sizeof(T)) = val;
    return val;
  }

or maybe

public:
  bool write(bool val) { return this->writeT(val); }
  void write(uint8_t val) { this->writeT(val); }
  void write(SkScalar val) { this->writeT(val); } 
  ... 
private:
  template <typename T>
  const T& writeT(const T& val) {
    T* out = sizeof(T) % 4 == 0
      ? this->reserve(sizeof(T))
      : this->reservePad(sizeof(T));
    *out = val;
    return val;
  }

[f(malita), 2014/02/10 16:10:35]

Ah, nice - overloading is not shun upon in Skia. I feel liberated!

+        *(T*)this->reserve(sizeof(value)) = value;
+    }
 
     bool writeBool(bool value) {
         this->write32(value);
@@ -104,26 +118,6 @@ public:
         *(int32_t*)this->reserve(sizeof(value)) = value;
     }
 
-    void writePtr(void* value) {
-        *(void**)this->reserve(sizeof(value)) = value;
-    }
-
-    void writeScalar(SkScalar value) {
-        *(SkScalar*)this->reserve(sizeof(value)) = value;
-    }
-
-    void writePoint(const SkPoint& pt) {
-        *(SkPoint*)this->reserve(sizeof(pt)) = pt;
-    }
-
-    void writeRect(const SkRect& rect) {
-        *(SkRect*)this->reserve(sizeof(rect)) = rect;
-    }
-
-    void writeIRect(const SkIRect& rect) {
-        *(SkIRect*)this->reserve(sizeof(rect)) = rect;
-    }
-
     void writeRRect(const SkRRect& rrect) {
         rrect.writeToMemory(this->reserve(SkRRect::kSizeInMemory));
     }
@@ -238,18 +232,20 @@ public:
     }
 
 private:
-    uint32_t& atOffset(size_t offset) {
+    template<typename T>
+    inline T& atOffset(size_t offset) const {
         SkASSERT(SkAlign4(offset) == offset);
-        const int count = SkToInt(offset/4);
-        SkASSERT(count < fCount);
+        const int count = SkToInt(offset / 4);
+        SkASSERT(offset + sizeof(T) <= (unsigned)fCount << 2);

[mtklein, 2014/02/06 21:38:16]

* 4 would be a bit more consistent with the rest of the file than << 2.

[f(malita), 2014/02/10 16:10:35]

Will do.

 
         if (count < this->externalCount()) {
-            return fExternal[count];
+            // A record should not span fExternal->fInternal.
+            SkASSERT(offset + sizeof(T) <= (unsigned)this->externalCount() << 2);
+            return *(T*)&fExternal[count];
         }
-        return fInternal[count - this->externalCount()];
+        return *(T*)&fInternal[count - this->externalCount()];
     }
 
-
     // Number of uint32_t written into fExternal.  <= fExternalLimit.
     int externalCount() const { return fCount - fInternal.count(); }