Templetized SkWriter32 readTAt() & overwriteTAt()

include/core/SkWriter32.h

 
 /*
  * Copyright 2008 The Android Open Source Project
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 
 #ifndef SkWriter32_DEFINED
@@ skipping 61 matching lines @@
         if (fInternal.isEmpty() && this->externalCount() + count <= fExternalLimit) {
             p = fExternal + fCount;
         } else {
             p = fInternal.append(count);
         }
 
         fCount += count;
         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);
         return value;
     }
 
     void writeInt(int32_t value) {
         this->write32(value);
     }
 
     void write8(int32_t value) {
         *(int32_t*)this->reserve(sizeof(value)) = value & 0xFF;
     }
 
     void write16(int32_t value) {
         *(int32_t*)this->reserve(sizeof(value)) = value & 0xFFFF;
     }
 
     void write32(int32_t value) {
         *(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));
     }
 
     void writePath(const SkPath& path) {
         size_t size = path.writeToMemory(NULL);
         SkASSERT(SkAlign4(size) == size);
         path.writeToMemory(this->reserve(size));
     }
 
@@ skipping 94 matching lines @@
             && stream->write(fInternal.begin(), fInternal.bytes());
     }
 
     // read from the stream, and write up to length bytes. Return the actual
     // number of bytes written.
     size_t readFromStream(SkStream* stream, size_t length) {
         return stream->read(this->reservePad(length), length);
     }
 
 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(); }
 
     int fCount;                     // Total number of uint32_t written.
     int fExternalLimit;             // Number of uint32_t we can write to fExternal.
     uint32_t* fExternal;            // Unmanaged memory block.
     SkTDArray<uint32_t> fInternal;  // Managed memory block.
 };
 
 /**
  *  Helper class to allocated SIZE bytes as part of the writer, and to provide
  *  that storage to the constructor as its initial storage buffer.
  *
  *  This wrapper ensures proper alignment rules are met for the storage.
  */
 template <size_t SIZE> class SkSWriter32 : public SkWriter32 {
 public:
     SkSWriter32() : SkWriter32(fData.fStorage, SIZE) {}
 
 private:
     union {
         void*   fPtrAlignment;
         double  fDoubleAlignment;
         char    fStorage[SIZE];
     } fData;
 };
 
 #endif