Guard against most unintentionally ephemeral SkAutoFoo instantiations.
I think I applied the trick everywhere possible. Limitations:
- can't be used with templated classes
- all constructors and destructors must be defined inline
A couple of the SkAutoFoo were unused in Skia, Chromium, and Android, so I
deleted them. This change caught the same bugs Cary found in SkPath, plus one
more in SampleApp.
BUG=
R=reed@google.com, caryclark@google.com
Author: mtklein@google.com
Review URL: https://codereview.chromium.org/72603005
git-svn-id: http://skia.googlecode.com/svn/trunk@12301 2bbb7eff-a529-9590-31e7-b0007b416f81
diff --git a/include/core/SkBitmap.h b/include/core/SkBitmap.h
index dd2d2cf..d9749f0 100644
--- a/include/core/SkBitmap.h
+++ b/include/core/SkBitmap.h
@@ -730,6 +730,7 @@
const SkBitmap& fBitmap;
bool fDidLock;
};
+#define SkAutoLockPixels(...) SK_REQUIRE_LOCAL_VAR(SkAutoLockPixels)
/** Helper class that performs the lock/unlockColors calls on a colortable.
The destructor will call unlockColors(false) if it has a bitmap's colortable
@@ -783,6 +784,7 @@
SkColorTable* fCTable;
const SkPMColor* fColors;
};
+#define SkAutoLockColors(...) SK_REQUIRE_LOCAL_VAR(SkAutoLockColors)
///////////////////////////////////////////////////////////////////////////////
diff --git a/include/core/SkCanvas.h b/include/core/SkCanvas.h
index c963edc..d831a5c 100644
--- a/include/core/SkCanvas.h
+++ b/include/core/SkCanvas.h
@@ -1158,6 +1158,7 @@
SkCanvas* fCanvas;
int fSaveCount;
};
+#define SkAutoCanvasRestore(...) SK_REQUIRE_LOCAL_VAR(SkAutoCanvasRestore)
/** Stack helper class to automatically open and close a comment block
*/
@@ -1179,5 +1180,6 @@
private:
SkCanvas* fCanvas;
};
+#define SkAutoCommentBlock(...) SK_REQUIRE_LOCAL_VAR(SkAutoCommentBlock)
#endif
diff --git a/include/core/SkMask.h b/include/core/SkMask.h
index 7155184..5cfef97 100644
--- a/include/core/SkMask.h
+++ b/include/core/SkMask.h
@@ -157,5 +157,6 @@
private:
uint8_t* fImage;
};
+#define SkAutoMaskFreeImage(...) SK_REQUIRE_LOCAL_VAR(SkAutoMaskFreeImage)
#endif
diff --git a/include/core/SkPicture.h b/include/core/SkPicture.h
index fa1cc49..e371c43 100644
--- a/include/core/SkPicture.h
+++ b/include/core/SkPicture.h
@@ -253,26 +253,6 @@
typedef SkRefCnt INHERITED;
};
-class SkAutoPictureRecord : SkNoncopyable {
-public:
- SkAutoPictureRecord(SkPicture* pict, int width, int height,
- uint32_t recordingFlags = 0) {
- fPicture = pict;
- fCanvas = pict->beginRecording(width, height, recordingFlags);
- }
- ~SkAutoPictureRecord() {
- fPicture->endRecording();
- }
-
- /** Return the canvas to draw into for recording into the picture.
- */
- SkCanvas* getRecordingCanvas() const { return fCanvas; }
-
-private:
- SkPicture* fPicture;
- SkCanvas* fCanvas;
-};
-
/**
* Subclasses of this can be passed to canvas.drawPicture. During the drawing
* of the picture, this callback will periodically be invoked. If its
diff --git a/include/core/SkRefCnt.h b/include/core/SkRefCnt.h
index 99df0c1..b010faf 100644
--- a/include/core/SkRefCnt.h
+++ b/include/core/SkRefCnt.h
@@ -237,11 +237,13 @@
private:
T* fObj;
};
+// Can't use the #define trick below to guard a bare SkAutoTUnref(...) because it's templated. :(
class SkAutoUnref : public SkAutoTUnref<SkRefCnt> {
public:
SkAutoUnref(SkRefCnt* obj) : SkAutoTUnref<SkRefCnt>(obj) {}
};
+#define SkAutoUnref(...) SK_REQUIRE_LOCAL_VAR(SkAutoUnref)
class SkAutoRef : SkNoncopyable {
public:
@@ -250,6 +252,7 @@
private:
SkRefCnt* fObj;
};
+#define SkAutoRef(...) SK_REQUIRE_LOCAL_VAR(SkAutoRef)
/** Wrapper class for SkRefCnt pointers. This manages ref/unref of a pointer to
a SkRefCnt (or subclass) object.
diff --git a/include/core/SkString.h b/include/core/SkString.h
index 5d97533..5629cc0 100644
--- a/include/core/SkString.h
+++ b/include/core/SkString.h
@@ -235,24 +235,6 @@
static Rec* RefRec(Rec*);
};
-class SkAutoUCS2 {
-public:
- SkAutoUCS2(const char utf8[]);
- ~SkAutoUCS2();
-
- /** This returns the number of ucs2 characters
- */
- int count() const { return fCount; }
-
- /** This returns a null terminated ucs2 string
- */
- const uint16_t* getUCS2() const { return fUCS2; }
-
-private:
- int fCount;
- uint16_t* fUCS2;
-};
-
/// Creates a new string and writes into it using a printf()-style format.
SkString SkStringPrintf(const char* format, ...);
diff --git a/include/core/SkThread.h b/include/core/SkThread.h
index 4a2499a..487c2bd 100644
--- a/include/core/SkThread.h
+++ b/include/core/SkThread.h
@@ -64,5 +64,6 @@
private:
SkBaseMutex* fMutex;
};
+#define SkAutoMutexAcquire(...) SK_REQUIRE_LOCAL_VAR(SkAutoMutexAcquire)
#endif
diff --git a/include/core/SkTime.h b/include/core/SkTime.h
index 7f3c2705..51616d4 100644
--- a/include/core/SkTime.h
+++ b/include/core/SkTime.h
@@ -60,5 +60,6 @@
SkMSec fNow;
SkMSec fMinToDump;
};
+#define SkAutoTime(...) SK_REQUIRE_LOCAL_VAR(SkAutoTime)
#endif
diff --git a/include/core/SkTypes.h b/include/core/SkTypes.h
index 219e51f..c30a8a2 100644
--- a/include/core/SkTypes.h
+++ b/include/core/SkTypes.h
@@ -149,6 +149,32 @@
*/
#define SK_MACRO_APPEND_LINE(name) SK_MACRO_CONCAT(name, __LINE__)
+/**
+ * For some classes, it's almost always an error to instantiate one without a name, e.g.
+ * {
+ * SkAutoMutexAcquire(&mutex);
+ * <some code>
+ * }
+ * In this case, the writer meant to hold mutex while the rest of the code in the block runs,
+ * but instead the mutex is acquired and then immediately released. The correct usage is
+ * {
+ * SkAutoMutexAcquire lock(&mutex);
+ * <some code>
+ * }
+ *
+ * To prevent callers from instantiating your class without a name, use SK_REQUIRE_LOCAL_VAR
+ * like this:
+ * class classname {
+ * <your class>
+ * };
+ * #define classname(...) SK_REQUIRE_LOCAL_VAR(classname)
+ *
+ * This won't work with templates, and you must inline the class' constructors and destructors.
+ * Take a look at SkAutoFree and SkAutoMalloc in this file for examples.
+ */
+#define SK_REQUIRE_LOCAL_VAR(classname) \
+ SK_COMPILE_ASSERT(false, missing_name_for_##classname)
+
///////////////////////////////////////////////////////////////////////
/**
@@ -432,6 +458,7 @@
SkAutoFree(const SkAutoFree&);
SkAutoFree& operator=(const SkAutoFree&);
};
+#define SkAutoFree(...) SK_REQUIRE_LOCAL_VAR(SkAutoFree)
/**
* Manage an allocated block of heap memory. This object is the sole manager of
@@ -518,6 +545,7 @@
void* fPtr;
size_t fSize; // can be larger than the requested size (see kReuse)
};
+#define SkAutoMalloc(...) SK_REQUIRE_LOCAL_VAR(SkAutoMalloc)
/**
* Manage an allocated block of memory. If the requested size is <= kSize, then
@@ -604,6 +632,7 @@
size_t fSize; // can be larger than the requested size (see kReuse)
uint32_t fStorage[(kSize + 3) >> 2];
};
+// Can't guard the constructor because it's a template class.
#endif /* C++ */
diff --git a/include/core/SkUtils.h b/include/core/SkUtils.h
index e29367d..c2dcad9 100644
--- a/include/core/SkUtils.h
+++ b/include/core/SkUtils.h
@@ -113,5 +113,6 @@
private:
const char* fLabel;
};
+#define SkAutoTrace(...) SK_REQUIRE_LOCAL_VAR(SkAutoTrace)
#endif
diff --git a/samplecode/SampleLayers.cpp b/samplecode/SampleLayers.cpp
index f72c519..483943d 100644
--- a/samplecode/SampleLayers.cpp
+++ b/samplecode/SampleLayers.cpp
@@ -61,7 +61,7 @@
SkPaint p;
p.setAlpha(0x88);
- SkAutoCanvasRestore(canvas, false);
+ SkAutoCanvasRestore ar2(canvas, false);
// create the layers
diff --git a/src/core/SkBlitter.cpp b/src/core/SkBlitter.cpp
index 7012cf3..dc7946a 100644
--- a/src/core/SkBlitter.cpp
+++ b/src/core/SkBlitter.cpp
@@ -780,6 +780,7 @@
void* fObj;
Proc fProc;
};
+#define SkAutoCallProc(...) SK_REQUIRE_LOCAL_VAR(SkAutoCallProc)
static void destroy_blitter(void* blitter) {
((SkBlitter*)blitter)->~SkBlitter();
diff --git a/src/core/SkCanvas.cpp b/src/core/SkCanvas.cpp
index 2efea72..0d5fccb 100644
--- a/src/core/SkCanvas.cpp
+++ b/src/core/SkCanvas.cpp
@@ -459,6 +459,7 @@
private:
SkBounder* fBounder;
};
+#define SkAutoBounderCommit(...) SK_REQUIRE_LOCAL_VAR(SkAutoBounderCommit)
#include "SkColorPriv.h"
diff --git a/src/core/SkComposeShader.cpp b/src/core/SkComposeShader.cpp
index 206608a..0d2d687 100644
--- a/src/core/SkComposeShader.cpp
+++ b/src/core/SkComposeShader.cpp
@@ -59,6 +59,7 @@
SkPaint* fPaint;
uint8_t fAlpha;
};
+#define SkAutoAlphaRestore(...) SK_REQUIRE_LOCAL_VAR(SkAutoAlphaRestore)
void SkComposeShader::flatten(SkFlattenableWriteBuffer& buffer) const {
this->INHERITED::flatten(buffer);
diff --git a/src/core/SkDescriptor.h b/src/core/SkDescriptor.h
index 79b086f..e71ff41 100644
--- a/src/core/SkDescriptor.h
+++ b/src/core/SkDescriptor.h
@@ -159,6 +159,7 @@
SkDescriptor* fDesc;
uint32_t fStorage[(kStorageSize + 3) >> 2];
};
+#define SkAutoDescriptor(...) SK_REQUIRE_LOCAL_VAR(SkAutoDescriptor)
#endif
diff --git a/src/core/SkDraw.cpp b/src/core/SkDraw.cpp
index 1124519..aa91df3 100644
--- a/src/core/SkDraw.cpp
+++ b/src/core/SkDraw.cpp
@@ -65,7 +65,13 @@
fStorage, sizeof(fStorage), drawCoverage);
}
- ~SkAutoBlitterChoose();
+ ~SkAutoBlitterChoose() {
+ if ((void*)fBlitter == (void*)fStorage) {
+ fBlitter->~SkBlitter();
+ } else {
+ SkDELETE(fBlitter);
+ }
+ }
SkBlitter* operator->() { return fBlitter; }
SkBlitter* get() const { return fBlitter; }
@@ -81,14 +87,7 @@
SkBlitter* fBlitter;
uint32_t fStorage[kBlitterStorageLongCount];
};
-
-SkAutoBlitterChoose::~SkAutoBlitterChoose() {
- if ((void*)fBlitter == (void*)fStorage) {
- fBlitter->~SkBlitter();
- } else {
- SkDELETE(fBlitter);
- }
-}
+#define SkAutoBlitterChoose(...) SK_REQUIRE_LOCAL_VAR(SkAutoBlitterChoose)
/**
* Since we are providing the storage for the shader (to avoid the perf cost
@@ -128,6 +127,7 @@
SkPaint fPaint; // copy of caller's paint (which we then modify)
uint32_t fStorage[kBlitterStorageLongCount];
};
+#define SkAutoBitmapShaderInstall(...) SK_REQUIRE_LOCAL_VAR(SkAutoBitmapShaderInstall)
///////////////////////////////////////////////////////////////////////////////
diff --git a/src/core/SkGlyphCache.h b/src/core/SkGlyphCache.h
index 8447337..52a8132 100644
--- a/src/core/SkGlyphCache.h
+++ b/src/core/SkGlyphCache.h
@@ -273,5 +273,6 @@
static bool DetachProc(const SkGlyphCache*, void*);
};
+#define SkAutoGlyphCache(...) SK_REQUIRE_LOCAL_VAR(SkAutoGlyphCache)
#endif
diff --git a/src/core/SkPath.cpp b/src/core/SkPath.cpp
index 5f53ce8..d25ec3c 100644
--- a/src/core/SkPath.cpp
+++ b/src/core/SkPath.cpp
@@ -56,6 +56,7 @@
SkPath* fPath;
bool fSaved;
};
+#define SkAutoDisableOvalCheck(...) SK_REQUIRE_LOCAL_VAR(SkAutoDisableOvalCheck)
class SkAutoDisableDirectionCheck {
public:
@@ -71,6 +72,7 @@
SkPath* fPath;
SkPath::Direction fSaved;
};
+#define SkAutoDisableDirectionCheck(...) SK_REQUIRE_LOCAL_VAR(SkAutoDisableDirectionCheck)
/* This guy's constructor/destructor bracket a path editing operation. It is
used when we know the bounds of the amount we are going to add to the path
@@ -125,6 +127,7 @@
fDegenerate = is_degenerate(*path);
}
};
+#define SkAutoPathBoundsUpdate(...) SK_REQUIRE_LOCAL_VAR(SkAutoPathBoundsUpdate)
////////////////////////////////////////////////////////////////////////////
diff --git a/src/core/SkRasterClip.h b/src/core/SkRasterClip.h
index e58a23b..0c27233 100644
--- a/src/core/SkRasterClip.h
+++ b/src/core/SkRasterClip.h
@@ -112,6 +112,7 @@
private:
const SkRasterClip& fRC;
};
+#define SkAutoRasterClipValidate(...) SK_REQUIRE_LOCAL_VAR(SkAutoRasterClipValidate)
#ifdef SK_DEBUG
#define AUTO_RASTERCLIP_VALIDATE(rc) SkAutoRasterClipValidate arcv(rc)
diff --git a/src/core/SkString.cpp b/src/core/SkString.cpp
index 4e5e204..7b3c265 100644
--- a/src/core/SkString.cpp
+++ b/src/core/SkString.cpp
@@ -626,27 +626,6 @@
///////////////////////////////////////////////////////////////////////////////
-SkAutoUCS2::SkAutoUCS2(const char utf8[]) {
- size_t len = strlen(utf8);
- fUCS2 = (uint16_t*)sk_malloc_throw((len + 1) * sizeof(uint16_t));
-
- uint16_t* dst = fUCS2;
- for (;;) {
- SkUnichar uni = SkUTF8_NextUnichar(&utf8);
- *dst++ = SkToU16(uni);
- if (uni == 0) {
- break;
- }
- }
- fCount = (int)(dst - fUCS2);
-}
-
-SkAutoUCS2::~SkAutoUCS2() {
- sk_free(fUCS2);
-}
-
-///////////////////////////////////////////////////////////////////////////////
-
SkString SkStringPrintf(const char* format, ...) {
SkString formattedOutput;
char buffer[kBufferSize];
diff --git a/src/ports/SkFontHost_win.cpp b/src/ports/SkFontHost_win.cpp
index be762ee..7416cb2 100755
--- a/src/ports/SkFontHost_win.cpp
+++ b/src/ports/SkFontHost_win.cpp
@@ -2153,6 +2153,7 @@
HFONT fFont;
HFONT fSavefont;
};
+#define SkAutoHDC(...) SK_REQUIRE_LOCAL_VAR(SkAutoHDC)
int LogFontTypeface::onCharsToGlyphs(const void* chars, Encoding encoding,
uint16_t userGlyphs[], int glyphCount) const
diff --git a/src/utils/mac/SkCreateCGImageRef.cpp b/src/utils/mac/SkCreateCGImageRef.cpp
index e931901..0677b7b 100644
--- a/src/utils/mac/SkCreateCGImageRef.cpp
+++ b/src/utils/mac/SkCreateCGImageRef.cpp
@@ -174,6 +174,7 @@
private:
CGPDFDocumentRef fDoc;
};
+#define SkAutoPDFRelease(...) SK_REQUIRE_LOCAL_VAR(SkAutoPDFRelease)
static void CGDataProviderReleaseData_FromMalloc(void*, const void* data,
size_t size) {