QFontEngine: use RAII for font_, face_ members

Wrap the pairs of (void *ptr, void (*dtor)(void*)) in essentially
a std::unique_ptr. This simplifies code and provides the correct
implicit destruction, so we can drop the explicit glyph-cache
clear()ing in ~QFontEngine(), leaving that job to ~QLinkedList.
A subsequent change will turn the QLinkedList into a C array, the
clearing of which would otherwise cause excessive code bloat.

Since we can't use std::unique_ptr, yet, provide a hand-rolled
replacement for now, marking it for replacement with unique_ptr
once we can use it. Make that a local type instead of providing
a Qt-wide unique_ptr so we don't accidentally lock ourselves into
a half-baked std clone we can't get rid of anymore.

To prepare unique_ptr use with the same type-erased deleter
(function pointer) as now, replace a nullptr destroy_function
with a no-op function, so ~unique_ptr doesn't crash when we
port to it later.

Because QFreetypeFace contains the same construct and shares
payloads with QFontEngine, use the Holder there, too.

Even saves 150b in text size on optimized GCC 5.3 AMD64 builds.

Change-Id: I5ca11a3e6e1ff9e06199124403d96e1b280f3eb2
Reviewed-by: Konstantin Ritt <ritt.ks@gmail.com>
Reviewed-by: Lars Knoll <lars.knoll@theqtcompany.com>

5 files changed, 59 insertions, 50 deletions

diff --git a/src/gui/text/qfontengine.cpp b/src/gui/text/qfontengine.cpp
index 7c0492bb1a..96f7e06a47 100644
--- a/src/gui/text/qfontengine.cpp
+++ b/src/gui/text/qfontengine.cpp
@@ -246,8 +246,8 @@ Q_AUTOTEST_EXPORT QList<QFontEngine *> QFontEngine_stopCollectingEngines()
 
 QFontEngine::QFontEngine(Type type)
     : m_type(type), ref(0),
-      font_(0), font_destroy_func(0),
-      face_(0), face_destroy_func(0),
+      font_(),
+      face_(),
       m_minLeftBearing(kBearingNotInitialized),
       m_minRightBearing(kBearingNotInitialized)
 {
@@ -269,17 +269,6 @@ QFontEngine::QFontEngine(Type type)
 
 QFontEngine::~QFontEngine()
 {
-    m_glyphCaches.clear();
-
-    if (font_ && font_destroy_func) {
-        font_destroy_func(font_);
-        font_ = 0;
-    }
-    if (face_ && face_destroy_func) {
-        face_destroy_func(face_);
-        face_ = 0;
-    }
-
 #ifdef QT_BUILD_INTERNAL
     if (enginesCollector)
         enginesCollector->removeOne(this);
@@ -334,10 +323,9 @@ void *QFontEngine::harfbuzzFont() const
         hbFont->x_scale = (((qint64)hbFont->x_ppem << 6) * 0x10000L + (emSquare >> 1)) / emSquare;
         hbFont->y_scale = (((qint64)hbFont->y_ppem << 6) * 0x10000L + (emSquare >> 1)) / emSquare;
 
-        font_ = (void *)hbFont;
-        font_destroy_func = free;
+        font_ = Holder(hbFont, free);
     }
-    return font_;
+    return font_.get();
 }
 
 void *QFontEngine::harfbuzzFace() const
@@ -357,10 +345,9 @@ void *QFontEngine::harfbuzzFace() const
         Q_CHECK_PTR(hbFace);
         hbFace->isSymbolFont = symbol;
 
-        face_ = (void *)hbFace;
-        face_destroy_func = hb_freeFace;
+        face_ = Holder(hbFace, hb_freeFace);
     }
-    return face_;
+    return face_.get();
 }
 
 bool QFontEngine::supportsScript(QChar::Script script) const
diff --git a/src/gui/text/qfontengine_ft.cpp b/src/gui/text/qfontengine_ft.cpp
index 779333351f..3207325755 100644
--- a/src/gui/text/qfontengine_ft.cpp
+++ b/src/gui/text/qfontengine_ft.cpp
@@ -246,9 +246,6 @@ QFreetypeFace *QFreetypeFace::getFace(const QFontEngine::FaceId &face_id,
         }
         newFreetype->face = face;
 
-        newFreetype->hbFace = 0;
-        newFreetype->hbFace_destroy_func = 0;
-
         newFreetype->ref.store(1);
         newFreetype->xsize = 0;
         newFreetype->ysize = 0;
@@ -300,10 +297,7 @@ QFreetypeFace *QFreetypeFace::getFace(const QFontEngine::FaceId &face_id,
 
 void QFreetypeFace::cleanup()
 {
-    if (hbFace && hbFace_destroy_func) {
-        hbFace_destroy_func(hbFace);
-        hbFace = 0;
-    }
+    hbFace.reset();
     FT_Done_Face(face);
     face = 0;
 }
@@ -686,6 +680,8 @@ bool QFontEngineFT::init(FaceId faceId, bool antialias, GlyphFormat format,
     return init(faceId, antialias, format, QFreetypeFace::getFace(faceId, fontData));
 }
 
+static void dont_delete(void*) {}
+
 bool QFontEngineFT::init(FaceId faceId, bool antialias, GlyphFormat format,
                          QFreetypeFace *freetypeFace)
 {
@@ -776,13 +772,13 @@ bool QFontEngineFT::init(FaceId faceId, bool antialias, GlyphFormat format,
     if (!freetype->hbFace) {
         faceData.user_data = face;
         faceData.get_font_table = ft_getSfntTable;
-        freetype->hbFace = harfbuzzFace();
-        freetype->hbFace_destroy_func = face_destroy_func;
+        (void)harfbuzzFace(); // populates face_
+        freetype->hbFace = std::move(face_);
     } else {
         Q_ASSERT(!face_);
-        face_ = freetype->hbFace;
     }
-    face_destroy_func = 0; // we share the HB face in QFreeTypeFace, so do not let ~QFontEngine() destroy it
+    // we share the HB face in QFreeTypeFace, so do not let ~QFontEngine() destroy it
+    face_ = Holder(freetype->hbFace.get(), dont_delete);
 
     unlockFace();
 
diff --git a/src/gui/text/qfontengine_ft_p.h b/src/gui/text/qfontengine_ft_p.h
index 1886fc67ba..f9b37ff039 100644
--- a/src/gui/text/qfontengine_ft_p.h
+++ b/src/gui/text/qfontengine_ft_p.h
@@ -121,8 +121,7 @@ private:
     QMutex _lock;
     QByteArray fontData;
 
-    void *hbFace;
-    qt_destroy_func_t hbFace_destroy_func;
+    QFontEngine::Holder hbFace;
 };
 
 // If this is exported this breaks compilation of the windows
diff --git a/src/gui/text/qfontengine_p.h b/src/gui/text/qfontengine_p.h
index 059b3df88e..1ef3a360d4 100644
--- a/src/gui/text/qfontengine_p.h
+++ b/src/gui/text/qfontengine_p.h
@@ -275,10 +275,45 @@ public:
     QAtomicInt ref;
     QFontDef fontDef;
 
-    mutable void *font_;
-    mutable qt_destroy_func_t font_destroy_func;
-    mutable void *face_;
-    mutable qt_destroy_func_t face_destroy_func;
+    class Holder { // replace by std::unique_ptr once available
+        void *ptr;
+        qt_destroy_func_t destroy_func;
+    public:
+        Holder() : ptr(nullptr), destroy_func(nullptr) {}
+        explicit Holder(void *p, qt_destroy_func_t d) : ptr(p), destroy_func(d) {}
+        ~Holder() { if (ptr && destroy_func) destroy_func(ptr); }
+        Holder(Holder &&other) Q_DECL_NOTHROW
+            : ptr(other.ptr),
+              destroy_func(other.destroy_func)
+        {
+            other.ptr = nullptr;
+            other.destroy_func = nullptr;
+        }
+        Holder &operator=(Holder &&other) Q_DECL_NOTHROW
+        { swap(other); return *this; }
+
+        void swap(Holder &other) Q_DECL_NOTHROW
+        {
+            qSwap(ptr, other.ptr);
+            qSwap(destroy_func, other.destroy_func);
+        }
+
+        void *get() const Q_DECL_NOTHROW { return ptr; }
+        void *release() Q_DECL_NOTHROW {
+            void *result = ptr;
+            ptr = nullptr;
+            destroy_func = nullptr;
+            return result;
+        }
+        void reset() Q_DECL_NOTHROW { Holder().swap(*this); }
+        qt_destroy_func_t get_deleter() const Q_DECL_NOTHROW { return destroy_func; }
+
+        bool operator!() const Q_DECL_NOTHROW { return !ptr; }
+    };
+
+    mutable Holder font_; // \ NOTE: Declared before m_glyphCaches, so font_, face_
+    mutable Holder face_; // / are destroyed _after_ m_glyphCaches is destroyed.
+
     struct FaceData {
         void *user_data;
         qt_get_font_table_func_t get_font_table;
diff --git a/src/gui/text/qharfbuzzng.cpp b/src/gui/text/qharfbuzzng.cpp
index e33b461401..55ef9f0d15 100644
--- a/src/gui/text/qharfbuzzng.cpp
+++ b/src/gui/text/qharfbuzzng.cpp
@@ -678,14 +678,10 @@ hb_face_t *hb_qt_face_get_for_engine(QFontEngine *fe)
 {
     Q_ASSERT(fe && fe->type() != QFontEngine::Multi);
 
-    if (Q_UNLIKELY(!fe->face_)) {
-        fe->face_ = _hb_qt_face_create(fe);
-        if (Q_UNLIKELY(!fe->face_))
-            return NULL;
-        fe->face_destroy_func = _hb_qt_face_release;
-    }
+    if (Q_UNLIKELY(!fe->face_))
+        fe->face_ = QFontEngine::Holder(_hb_qt_face_create(fe), _hb_qt_face_release);
 
-    return static_cast<hb_face_t *>(fe->face_);
+    return static_cast<hb_face_t *>(fe->face_.get());
 }
 
 
@@ -728,14 +724,10 @@ hb_font_t *hb_qt_font_get_for_engine(QFontEngine *fe)
 {
     Q_ASSERT(fe && fe->type() != QFontEngine::Multi);
 
-    if (Q_UNLIKELY(!fe->font_)) {
-        fe->font_ = _hb_qt_font_create(fe);
-        if (Q_UNLIKELY(!fe->font_))
-            return NULL;
-        fe->font_destroy_func = _hb_qt_font_release;
-    }
+    if (Q_UNLIKELY(!fe->font_))
+        fe->font_ = QFontEngine::Holder(_hb_qt_font_create(fe), _hb_qt_font_release);
 
-    return static_cast<hb_font_t *>(fe->font_);
+    return static_cast<hb_font_t *>(fe->font_.get());
 }
 
 QT_END_NAMESPACE