diff options
author | commit-bot@chromium.org <commit-bot@chromium.org@2bbb7eff-a529-9590-31e7-b0007b416f81> | 2014-04-24 15:33:48 +0000 |
---|---|---|
committer | commit-bot@chromium.org <commit-bot@chromium.org@2bbb7eff-a529-9590-31e7-b0007b416f81> | 2014-04-24 15:33:48 +0000 |
commit | f0ae5e471b759f18b614a8e0928c9151947de04c (patch) | |
tree | 9f222c09a5732d4efa3d81d8771d6c5768792c86 | |
parent | 732bd66ac2aea4bacd1e7e8f33628f7efb50f1d0 (diff) |
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
-rw-r--r-- | src/record/SkRecord.h | 37 | ||||
-rw-r--r-- | src/record/SkRecordOpts.cpp | 30 | ||||
-rw-r--r-- | src/record/SkRecords.h | 10 |
3 files changed, 48 insertions, 29 deletions
diff --git a/src/record/SkRecord.h b/src/record/SkRecord.h index ddf1b3d95f..b1e4ae24f8 100644 --- a/src/record/SkRecord.h +++ b/src/record/SkRecord.h @@ -82,20 +82,31 @@ public: // 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 @@ private: // // 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 @@ private: 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: } 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 3cf135fb7e..4b35b33a3c 100644 --- a/src/record/SkRecordOpts.cpp +++ b/src/record/SkRecordOpts.cpp @@ -18,10 +18,6 @@ void SkRecordOptimize(SkRecord* record) { 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 @@ void SaveRestoreNooper::operator()(SkRecords::Restore* r) { 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 @@ void CullAnnotator::operator()(SkRecords::PopCull* pop) { 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 @@ void StrengthReducer::operator()(SkRecords::DrawPosText* r) { 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 @@ void TextBounder::operator()(SkRecords::DrawPosTextH* r) { 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 @@ void SkRecordBoundDrawPosTextH(SkRecord* record) { TextBounder bounder(record); run_pass(bounder, record); } - -#undef REPLACE diff --git a/src/record/SkRecords.h b/src/record/SkRecords.h index 8b96e8d913..bfa15496f3 100644 --- a/src/record/SkRecords.h +++ b/src/record/SkRecords.h @@ -133,7 +133,12 @@ template <typename T> 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 @@ private: // 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: |