Proof of adoption in SkRecord::replace.
It used to be an unenforced requirement that callers take ownership of
the command which was replaced when calling SkRecord::replace. Now we
can enforce it, by splitting replace into two modes:
- T* replace(i): always destroys the existing command for you
- T* replace(i, proofOfAdoption): proofOfAdoption is checked to make
sure the caller has adopted the existing command before replacing it.
BUG=skia:2378
R=fmalita@chromium.org, mtklein@google.com
Author: mtklein@chromium.org
Review URL: https://codereview.chromium.org/248053008
git-svn-id: http://skia.googlecode.com/svn/trunk@14352 2bbb7eff-a529-9590-31e7-b0007b416f81
diff --git a/src/record/SkRecord.h b/src/record/SkRecord.h
index ddf1b3d..b1e4ae2 100644
--- a/src/record/SkRecord.h
+++ b/src/record/SkRecord.h
@@ -82,20 +82,31 @@
// Replace the i-th command with a new command of type T.
// You are expected to placement new an object of type T onto this pointer.
- // References to the old command remain valid for the life of the SkRecord, but
- // you must destroy the old command. (It's okay to destroy it first before calling replace.)
+ // References to the original command are invalidated.
template <typename T>
T* replace(unsigned i) {
SkASSERT(i < this->count());
+
+ Destroyer destroyer;
+ this->mutate(i, destroyer);
+
fTypes[i] = T::kType;
return fRecords[i].set(this->alloc<T>());
}
- // A mutator that can be used with replace to destroy canvas commands.
- struct Destroyer {
- template <typename T>
- void operator()(T* record) { record->~T(); }
- };
+ // Replace the i-th command with a new command of type T.
+ // You are expected to placement new an object of type T onto this pointer.
+ // You must show proof that you've already adopted the existing command.
+ template <typename T, typename Existing>
+ T* replace(unsigned i, const SkRecords::Adopted<Existing>& proofOfAdoption) {
+ SkASSERT(i < this->count());
+
+ SkASSERT(Existing::kType == fTypes[i]);
+ SkASSERT(proofOfAdoption == fRecords[i].ptr<Existing>());
+
+ fTypes[i] = T::kType;
+ return fRecords[i].set(this->alloc<T>());
+ }
private:
// Implementation notes!
@@ -134,6 +145,11 @@
//
// The cost to append a T into this structure is 1 + sizeof(void*) + sizeof(T).
+ // A mutator that can be used with replace to destroy canvas commands.
+ struct Destroyer {
+ template <typename T>
+ void operator()(T* record) { record->~T(); }
+ };
// Logically the same as SkRecords::Type, but packed into 8 bits.
struct Type8 {
@@ -157,6 +173,10 @@
return ptr;
}
+ // Get the data in fAlloc, assuming it's of type T.
+ template <typename T>
+ T* ptr() const { return (T*)fPtr; }
+
// Visit this record with functor F (see public API above) assuming the record we're
// pointing to has this type.
template <typename F>
@@ -176,9 +196,6 @@
}
private:
- template <typename T>
- T* ptr() const { return (T*)fPtr; }
-
void* fPtr;
};
diff --git a/src/record/SkRecordOpts.cpp b/src/record/SkRecordOpts.cpp
index 3cf135f..4b35b33 100644
--- a/src/record/SkRecordOpts.cpp
+++ b/src/record/SkRecordOpts.cpp
@@ -18,10 +18,6 @@
SkRecordBoundDrawPosTextH(record);
}
-// Streamline replacing one command with another.
-#define REPLACE(record, index, T, ...) \
- SkNEW_PLACEMENT_ARGS(record->replace<SkRecords::T>(index), SkRecords::T, (__VA_ARGS__))
-
namespace {
// Convenience base class to share some common implementation code.
@@ -81,10 +77,8 @@
if (fSave != kInactive) {
// Remove everything between the save and restore, inclusive on both sides.
fChanged = true;
- SkRecord::Destroyer destroyer;
for (unsigned i = fSave; i <= this->index(); i++) {
- fRecord->mutate(i, destroyer);
- REPLACE(fRecord, i, NoOp);
+ fRecord->replace<SkRecords::NoOp>(i);
}
fSave = kInactive;
}
@@ -124,8 +118,9 @@
SkASSERT(this->index() > push.index);
unsigned skip = this->index() - push.index;
- // PairedPushCull adopts push.command.
- REPLACE(fRecord, push.index, PairedPushCull, push.command, skip);
+ SkRecords::Adopted<SkRecords::PushCull> adopted(push.command);
+ SkNEW_PLACEMENT_ARGS(fRecord->replace<SkRecords::PairedPushCull>(push.index, adopted),
+ SkRecords::PairedPushCull, (&adopted, skip));
}
// Replaces DrawPosText with DrawPosTextH when all Y coordinates are equal.
@@ -164,10 +159,11 @@
scalars[i/2] = scalars[i];
}
- SkRecord::Destroyer destroyer;
- fRecord->mutate(this->index(), destroyer);
- REPLACE(fRecord, this->index(),
- DrawPosTextH, (char*)r->text, r->byteLength, scalars, firstY, r->paint);
+ // Extend lifetime of r to the end of the method so we can copy its parts.
+ SkRecords::Adopted<SkRecords::DrawPosText> adopted(r);
+ SkNEW_PLACEMENT_ARGS(fRecord->replace<SkRecords::DrawPosTextH>(this->index(), adopted),
+ SkRecords::DrawPosTextH,
+ (r->text, r->byteLength, scalars, firstY, r->paint));
}
@@ -204,8 +200,10 @@
bounds.set(0, r->y - buffer, SK_Scalar1, r->y + buffer);
SkRect adjusted = r->paint.computeFastBounds(bounds, &bounds);
- // BoundedDrawPosTextH adopts r.
- REPLACE(fRecord, this->index(), BoundedDrawPosTextH, r, adjusted.fTop, adjusted.fBottom);
+ SkRecords::Adopted<SkRecords::DrawPosTextH> adopted(r);
+ SkNEW_PLACEMENT_ARGS(fRecord->replace<SkRecords::BoundedDrawPosTextH>(this->index(), adopted),
+ SkRecords::BoundedDrawPosTextH,
+ (&adopted, adjusted.fTop, adjusted.fBottom));
}
template <typename Pass>
@@ -242,5 +240,3 @@
TextBounder bounder(record);
run_pass(bounder, record);
}
-
-#undef REPLACE
diff --git a/src/record/SkRecords.h b/src/record/SkRecords.h
index 8b96e8d..bfa1549 100644
--- a/src/record/SkRecords.h
+++ b/src/record/SkRecords.h
@@ -133,7 +133,12 @@
class Adopted : SkNoncopyable {
public:
Adopted(T* ptr) : fPtr(ptr) { SkASSERT(fPtr); }
- ~Adopted() { fPtr->~T(); }
+ Adopted(Adopted* source) {
+ // Transfer ownership from source to this.
+ fPtr = source->fPtr;
+ source->fPtr = NULL;
+ }
+ ~Adopted() { if (fPtr) fPtr->~T(); }
ACT_AS_PTR(fPtr);
private:
@@ -142,9 +147,10 @@
// PODArray doesn't own the pointer's memory, and we assume the data is POD.
template <typename T>
-class PODArray : SkNoncopyable {
+class PODArray {
public:
PODArray(T* ptr) : fPtr(ptr) {}
+ // Default copy and assign.
ACT_AS_PTR(fPtr);
private: