From 7b7b78cb9f1bd8b57fb849e5cae7f477e84d3cf6 Mon Sep 17 00:00:00 2001 From: Ben Wagner Date: Wed, 18 Mar 2020 13:20:32 -0400 Subject: [PATCH] Make SkPDFUnion a better variant. SkPDFUnion is basically a tagged union but does some odd things around unique_ptr and SkString. Clean up the implementation around these. This also fixes a number of warnings given by gcc 9. Change-Id: Iaf58b30c03f172e96a28826ddaa616bf9f655f71 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/277613 Reviewed-by: Herb Derby Commit-Queue: Ben Wagner --- src/pdf/SkPDFTypes.cpp | 128 +++++++++++++++++++++-------------------- src/pdf/SkPDFUnion.h | 34 ++++------- 2 files changed, 76 insertions(+), 86 deletions(-) diff --git a/src/pdf/SkPDFTypes.cpp b/src/pdf/SkPDFTypes.cpp index ec92b82e64..484f4f2adb 100644 --- a/src/pdf/SkPDFTypes.cpp +++ b/src/pdf/SkPDFTypes.cpp @@ -21,72 +21,76 @@ //////////////////////////////////////////////////////////////////////////////// -SkPDFUnion::SkPDFUnion(Type t) : fType(t) {} -SkPDFUnion::SkPDFUnion(Type t, int32_t v) : fIntValue (v), fType(t) {} -SkPDFUnion::SkPDFUnion(Type t, bool v) : fBoolValue (v), fType(t) {} -SkPDFUnion::SkPDFUnion(Type t, SkScalar v) : fScalarValue (v), fType(t) {} -SkPDFUnion::SkPDFUnion(Type t, SkString v) : fType(t) { fSkString.init(std::move(v)); } +SkPDFUnion::SkPDFUnion(Type t, int32_t v) : fIntValue (v) , fType(t) {} +SkPDFUnion::SkPDFUnion(Type t, bool v) : fBoolValue (v) , fType(t) {} +SkPDFUnion::SkPDFUnion(Type t, SkScalar v) : fScalarValue (v) , fType(t) {} +SkPDFUnion::SkPDFUnion(Type t, const char* v) : fStaticString (v) , fType(t) {} +SkPDFUnion::SkPDFUnion(Type t, SkString v) : fSkString(std::move(v)), fType(t) {} +SkPDFUnion::SkPDFUnion(Type t, PDFObject v) : fObject (std::move(v)), fType(t) {} SkPDFUnion::~SkPDFUnion() { switch (fType) { case Type::kNameSkS: case Type::kStringSkS: - fSkString.destroy(); + fSkString.~SkString(); return; case Type::kObject: - SkASSERT(fObject); - delete fObject; + fObject.~PDFObject(); return; default: return; } } -SkPDFUnion& SkPDFUnion::operator=(SkPDFUnion&& other) { - if (this != &other) { +SkPDFUnion::SkPDFUnion(SkPDFUnion&& that) : fType(that.fType) { + SkASSERT(this != &that); + + switch (fType) { + case Type::kDestroyed: + break; + case Type::kInt: + case Type::kColorComponent: + case Type::kRef: + fIntValue = that.fIntValue; + break; + case Type::kBool: + fBoolValue = that.fBoolValue; + break; + case Type::kColorComponentF: + case Type::kScalar: + fScalarValue = that.fScalarValue; + break; + case Type::kName: + case Type::kString: + fStaticString = that.fStaticString; + break; + case Type::kNameSkS: + case Type::kStringSkS: + new (&fSkString) SkString(std::move(that.fSkString)); + break; + case Type::kObject: + new (&fObject) PDFObject(std::move(that.fObject)); + break; + default: + SkDEBUGFAIL("SkPDFUnion::SkPDFUnion with bad type"); + } + that.fType = Type::kDestroyed; +} + +SkPDFUnion& SkPDFUnion::operator=(SkPDFUnion&& that) { + if (this != &that) { this->~SkPDFUnion(); - new (this) SkPDFUnion(std::move(other)); + new (this) SkPDFUnion(std::move(that)); } return *this; } -SkPDFUnion::SkPDFUnion(SkPDFUnion&& other) { - SkASSERT(this != &other); - memcpy(this, &other, sizeof(*this)); - other.fType = Type::kDestroyed; -} - -#if 0 -SkPDFUnion SkPDFUnion::copy() const { - SkPDFUnion u(fType); - memcpy(&u, this, sizeof(u)); - switch (fType) { - case Type::kNameSkS: - case Type::kStringSkS: - u.fSkString.init(fSkString.get()); - return u; - case Type::kObject: - SkRef(u.fObject); - return u; - default: - return u; - } -} -SkPDFUnion& SkPDFUnion::operator=(const SkPDFUnion& other) { - return *this = other.copy(); -} -SkPDFUnion::SkPDFUnion(const SkPDFUnion& other) { - *this = other.copy(); -} -#endif - bool SkPDFUnion::isName() const { return Type::kName == fType || Type::kNameSkS == fType; } #ifdef SK_DEBUG -// Most names need no escaping. Such names are handled as static -// const strings. +// Most names need no escaping. Such names are handled as static const strings. bool is_valid_name(const char* n) { static const char kControlChars[] = "/%()<>[]{}"; while (*n) { @@ -99,8 +103,7 @@ bool is_valid_name(const char* n) { } #endif // SK_DEBUG -// Given an arbitrary string, write it as a valid name (not including -// leading slash). +// Given an arbitrary string, write it as a valid name (not including leading slash). static void write_name_escaped(SkWStream* o, const char* name) { static const char kToEscape[] = "#/%()<>[]{}"; for (const uint8_t* n = reinterpret_cast(name); *n; ++n) { @@ -190,10 +193,10 @@ void SkPDFUnion::emitObject(SkWStream* stream) const { return; case Type::kNameSkS: stream->writeText("/"); - write_name_escaped(stream, fSkString.get().c_str()); + write_name_escaped(stream, fSkString.c_str()); return; case Type::kStringSkS: - write_string(stream, fSkString.get().c_str(), fSkString.get().size()); + write_string(stream, fSkString.c_str(), fSkString.size()); return; case Type::kObject: fObject->emitObject(stream); @@ -208,14 +211,16 @@ void SkPDFUnion::emitObject(SkWStream* stream) const { } } -SkPDFUnion SkPDFUnion::Int(int32_t value) { return SkPDFUnion(Type::kInt, value); } +SkPDFUnion SkPDFUnion::Int(int32_t value) { + return SkPDFUnion(Type::kInt, value); +} SkPDFUnion SkPDFUnion::ColorComponent(uint8_t value) { - return SkPDFUnion(Type::kColorComponent, (int32_t)value); + return SkPDFUnion(Type::kColorComponent, SkTo(value)); } SkPDFUnion SkPDFUnion::ColorComponentF(float value) { - return SkPDFUnion(Type::kColorComponentF, (SkScalar)value); + return SkPDFUnion(Type::kColorComponentF, SkFloatToScalar(value)); } SkPDFUnion SkPDFUnion::Bool(bool value) { @@ -227,33 +232,32 @@ SkPDFUnion SkPDFUnion::Scalar(SkScalar value) { } SkPDFUnion SkPDFUnion::Name(const char* value) { - SkPDFUnion u(Type::kName); SkASSERT(value); SkASSERT(is_valid_name(value)); - u.fStaticString = value; - return u; + return SkPDFUnion(Type::kName, value); } SkPDFUnion SkPDFUnion::String(const char* value) { - SkPDFUnion u(Type::kString); SkASSERT(value); - u.fStaticString = value; - return u; + return SkPDFUnion(Type::kString, value); } -SkPDFUnion SkPDFUnion::Name(SkString s) { return SkPDFUnion(Type::kNameSkS, std::move(s)); } +SkPDFUnion SkPDFUnion::Name(SkString s) { + return SkPDFUnion(Type::kNameSkS, std::move(s)); +} -SkPDFUnion SkPDFUnion::String(SkString s) { return SkPDFUnion(Type::kStringSkS, std::move(s)); } +SkPDFUnion SkPDFUnion::String(SkString s) { + return SkPDFUnion(Type::kStringSkS, std::move(s)); +} SkPDFUnion SkPDFUnion::Object(std::unique_ptr objSp) { - SkPDFUnion u(Type::kObject); SkASSERT(objSp.get()); - u.fObject = objSp.release(); // take ownership into union{} - return u; + return SkPDFUnion(Type::kObject, std::move(objSp)); } SkPDFUnion SkPDFUnion::Ref(SkPDFIndirectReference ref) { - return SkASSERT(ref.fValue > 0), SkPDFUnion(Type::kRef, (int32_t)ref.fValue); + SkASSERT(ref.fValue > 0); + return SkPDFUnion(Type::kRef, SkTo(ref.fValue)); } //////////////////////////////////////////////////////////////////////////////// diff --git a/src/pdf/SkPDFUnion.h b/src/pdf/SkPDFUnion.h index 9eeec20480..f79e4c5ec0 100644 --- a/src/pdf/SkPDFUnion.h +++ b/src/pdf/SkPDFUnion.h @@ -5,20 +5,6 @@ #include "src/pdf/SkPDFTypes.h" -template -class SkStorageFor { -public: - const T& get() const { return *reinterpret_cast(&fStore); } - T& get() { return *reinterpret_cast(&fStore); } - // Up to caller to keep track of status. - template void init(Args&&... args) { - new (&this->get()) T(std::forward(args)...); - } - void destroy() { this->get().~T(); } -private: - typename std::aligned_storage::type fStore; -}; - // Exposed for unit testing. void SkPDFWriteString(SkWStream* wStream, const char* cin, size_t len); @@ -31,10 +17,10 @@ void SkPDFWriteString(SkWStream* wStream, const char* cin, size_t len); */ class SkPDFUnion { public: - // Move contstructor and assignment operator destroy the argument + // Move constructor and assignment operator destroy the argument // and steal their references (if needed). - SkPDFUnion(SkPDFUnion&& other); - SkPDFUnion& operator=(SkPDFUnion&& other); + SkPDFUnion(SkPDFUnion&&); + SkPDFUnion& operator=(SkPDFUnion&&); ~SkPDFUnion(); @@ -87,17 +73,17 @@ public: bool isName() const; private: + using PDFObject = std::unique_ptr; union { int32_t fIntValue; bool fBoolValue; SkScalar fScalarValue; const char* fStaticString; - SkStorageFor fSkString; - SkPDFObject* fObject; + SkString fSkString; + PDFObject fObject; }; enum class Type : char { - /** It is an error to call emitObject() or addResources() on an - kDestroyed object. */ + /** It is an error to call emitObject() or addResources() on an kDestroyed object. */ kDestroyed = 0, kInt, kColorComponent, @@ -113,13 +99,13 @@ private: }; Type fType; - SkPDFUnion(Type); SkPDFUnion(Type, int32_t); SkPDFUnion(Type, bool); SkPDFUnion(Type, SkScalar); + SkPDFUnion(Type, const char*); SkPDFUnion(Type, SkString); - // We do not now need copy constructor and copy assignment, so we - // will disable this functionality. + SkPDFUnion(Type, PDFObject); + SkPDFUnion& operator=(const SkPDFUnion&) = delete; SkPDFUnion(const SkPDFUnion&) = delete; };