Le 21/02/2017 à 00:08, Guillaume Munch a écrit :
Could you do the backport to stable? :)
What about that? Please check especially the code related to
LYX_USE_CXX11 in Cache.h. For the caching, I re-read the code to make
sure that my hand-merging was correct, I hope I did not miss anything.
BTW, why don't you use Cache::contains in getLayout like you do for
other cache uses?
JMarc
From 30b62647ba2dc111aeb4425292e92994cf96f931 Mon Sep 17 00:00:00 2001
From: Guillaume Munch <g...@lyx.org>
Date: Mon, 20 Feb 2017 23:59:24 +0100
Subject: [PATCH] Introduce support/Cache.h
Useful to cache copies of objects, including shared_ptrs. No risks of dangling
pointer, and avoid naked pointers in the source.
Fix memory leak when compiling with Qt5.
As part as the backport to stable, this code has been change to work
with C++98.
(cherry picked from commit 33b696c8acf2e64b44d449180781de6dbc203709)
(cherry picked from commit e04079aa528ecbf4a8e39ed2b19c3cb50174e151)
(cherry picked from commit 5211ca52cac2ad7a6669d15c39f2cee172d18323)
---
src/frontends/qt4/GuiFontMetrics.cpp | 126 +++++++++++++++-------------------
src/frontends/qt4/GuiFontMetrics.h | 46 +++----------
src/frontends/qt4/GuiPainter.cpp | 3 +-
src/support/Cache.h | 88 ++++++++++++++++++++++++
src/support/Makefile.am | 1 +
5 files changed, 159 insertions(+), 105 deletions(-)
create mode 100644 src/support/Cache.h
diff --git a/src/frontends/qt4/GuiFontMetrics.cpp b/src/frontends/qt4/GuiFontMetrics.cpp
index 5bb99aa..80ff819 100644
--- a/src/frontends/qt4/GuiFontMetrics.cpp
+++ b/src/frontends/qt4/GuiFontMetrics.cpp
@@ -24,14 +24,11 @@
#include "support/convert.h"
#include "support/lassert.h"
-#ifdef CACHE_SOME_METRICS
#include <QByteArray>
-#endif
using namespace std;
using namespace lyx::support;
-#ifdef CACHE_SOME_METRICS
namespace std {
/*
@@ -46,11 +43,28 @@ uint qHash(lyx::docstring const & s)
}
}
-#endif
namespace lyx {
namespace frontend {
+
+/*
+ * Limit (strwidth|breakat)_cache_ size to 512kB of string data.
+ * Limit qtextlayout_cache_ size to 500 elements (we do not know the
+ * size of the QTextLayout objects anyway).
+ * Note that all these numbers are arbitrary.
+ * Also, setting size to 0 is tantamount to disabling the cache.
+ */
+int cache_metrics_width_size = 1 << 19;
+int cache_metrics_breakat_size = 1 << 19;
+// Qt 5.x already has its own caching of QTextLayout objects
+#if (QT_VERSION < 0x050000)
+int cache_metrics_qtextlayout_size = 500;
+#else
+int cache_metrics_qtextlayout_size = 0;
+#endif
+
+
namespace {
/**
* Convert a UCS4 character into a QChar.
@@ -73,23 +87,11 @@ inline QChar const ucs4_to_qchar(char_type const ucs4)
} // anon namespace
-/*
- * Limit (strwidth|breakat)_cache_ size to 512kB of string data.
- * Limit qtextlayout_cache_ size to 500 elements (we do not know the
- * size of the QTextLayout objects anyway).
- * Note that all these numbers are arbitrary.
- */
GuiFontMetrics::GuiFontMetrics(QFont const & font)
- : font_(font), metrics_(font, 0)
-#ifdef CACHE_METRICS_WIDTH
- , strwidth_cache_(1 << 19)
-#endif
-#ifdef CACHE_METRICS_BREAKAT
- , breakat_cache_(1 << 19)
-#endif
-#ifdef CACHE_METRICS_QTEXTLAYOUT
- , qtextlayout_cache_(500)
-#endif
+ : font_(font), metrics_(font, 0),
+ strwidth_cache_(cache_metrics_width_size),
+ breakat_cache_(cache_metrics_breakat_size),
+ qtextlayout_cache_(cache_metrics_qtextlayout_size)
{
}
@@ -174,11 +176,8 @@ int GuiFontMetrics::rbearing(char_type c) const
int GuiFontMetrics::width(docstring const & s) const
{
-#ifdef CACHE_METRICS_WIDTH
- int * pw = strwidth_cache_[s];
- if (pw)
- return *pw;
-#endif
+ if (strwidth_cache_.contains(s))
+ return strwidth_cache_[s];
/* For some reason QMetrics::width returns a wrong value with Qt5
* with some arabic text. OTOH, QTextLayout is broken for single
* characters with null width (like \not in mathed). Also, as a
@@ -200,9 +199,7 @@ int GuiFontMetrics::width(docstring const & s) const
tl.endLayout();
w = int(line.naturalTextWidth());
}
-#ifdef CACHE_METRICS_WIDTH
- strwidth_cache_.insert(s, new int(w), s.size() * sizeof(char_type));
-#endif
+ strwidth_cache_.insert(s, w, s.size() * sizeof(char_type));
return w;
}
@@ -225,31 +222,26 @@ int GuiFontMetrics::signedWidth(docstring const & s) const
}
-QTextLayout const *
+shared_ptr<QTextLayout const>
GuiFontMetrics::getTextLayout(docstring const & s, bool const rtl,
double const wordspacing) const
{
- QTextLayout * ptl;
-#ifdef CACHE_METRICS_QTEXTLAYOUT
- docstring const s_cache = s + (rtl ? "r" : "l") + convert<docstring>(wordspacing);
- ptl = qtextlayout_cache_[s_cache];
- if (!ptl) {
-#endif
- ptl = new QTextLayout();
- ptl->setCacheEnabled(true);
- ptl->setText(toqstr(s));
- QFont copy = font_;
- copy.setWordSpacing(wordspacing);
- ptl->setFont(copy);
- // Note that both setFlags and the enums are undocumented
- ptl->setFlags(rtl ? Qt::TextForceRightToLeft : Qt::TextForceLeftToRight);
- ptl->beginLayout();
- ptl->createLine();
- ptl->endLayout();
-#ifdef CACHE_METRICS_QTEXTLAYOUT
- qtextlayout_cache_.insert(s_cache, ptl);
- }
-#endif
+ docstring const s_cache =
+ s + (rtl ? "r" : "l") + convert<docstring>(wordspacing);
+ if (shared_ptr<QTextLayout const> ptl = qtextlayout_cache_[s_cache])
+ return ptl;
+ shared_ptr<QTextLayout> const ptl = make_shared<QTextLayout>();
+ ptl->setCacheEnabled(true);
+ ptl->setText(toqstr(s));
+ QFont copy = font_;
+ copy.setWordSpacing(wordspacing);
+ ptl->setFont(copy);
+ // Note that both setFlags and the enums are undocumented
+ ptl->setFlags(rtl ? Qt::TextForceRightToLeft : Qt::TextForceLeftToRight);
+ ptl->beginLayout();
+ ptl->createLine();
+ ptl->endLayout();
+ qtextlayout_cache_.insert(s_cache, ptl);
return ptl;
}
@@ -259,7 +251,7 @@ int GuiFontMetrics::pos2x(docstring const & s, int pos, bool const rtl,
{
if (pos <= 0)
pos = 0;
- QTextLayout const * tl = getTextLayout(s, rtl, wordspacing);
+ shared_ptr<QTextLayout const> tl = getTextLayout(s, rtl, wordspacing);
/* Since QString is UTF-16 and docstring is UCS-4, the offsets may
* not be the same when there are high-plan unicode characters
* (bug #10443).
@@ -272,7 +264,7 @@ int GuiFontMetrics::pos2x(docstring const & s, int pos, bool const rtl,
int GuiFontMetrics::x2pos(docstring const & s, int & x, bool const rtl,
double const wordspacing) const
{
- QTextLayout const * tl = getTextLayout(s, rtl, wordspacing);
+ shared_ptr<QTextLayout const> tl = getTextLayout(s, rtl, wordspacing);
int const qpos = tl->lineForTextPosition(0).xToCursor(x);
// correct x value to the actual cursor position.
x = static_cast<int>(tl->lineForTextPosition(0).cursorToX(qpos));
@@ -301,7 +293,7 @@ int GuiFontMetrics::x2pos(docstring const & s, int & x, bool const rtl,
-pair<int, int> *
+pair<int, int>
GuiFontMetrics::breakAt_helper(docstring const & s, int const x,
bool const rtl, bool const force) const
{
@@ -346,7 +338,7 @@ GuiFontMetrics::breakAt_helper(docstring const & s, int const x,
tl.createLine();
tl.endLayout();
if ((force && line.textLength() == offset) || int(line.naturalTextWidth()) > x)
- return new pair<int, int>(-1, -1);
+ return make_pair(-1, -1);
/* Since QString is UTF-16 and docstring is UCS-4, the offsets may
* not be the same when there are high-plan unicode characters
* (bug #10443).
@@ -371,7 +363,7 @@ GuiFontMetrics::breakAt_helper(docstring const & s, int const x,
LASSERT(len > 0 || qlen == 0, /**/);
#endif
// The -1 is here to account for the leading zerow_nbsp.
- return new pair<int, int>(len, int(line.naturalTextWidth()));
+ return make_pair(len, int(line.naturalTextWidth()));
}
@@ -379,25 +371,21 @@ bool GuiFontMetrics::breakAt(docstring & s, int & x, bool const rtl, bool const
{
if (s.empty())
return false;
- pair<int, int> * pp;
-#ifdef CACHE_METRICS_BREAKAT
- docstring const s_cache = s + convert<docstring>(x) + (rtl ? "r" : "l") + (force ? "f" : "w");
- pp = breakat_cache_[s_cache];
- if (!pp) {
-#endif
+ docstring const s_cache =
+ s + convert<docstring>(x) + (rtl ? "r" : "l") + (force ? "f" : "w");
+ pair<int, int> pp;
+
+ if (breakat_cache_.contains(s_cache))
+ pp = breakat_cache_[s_cache];
+ else {
pp = breakAt_helper(s, x, rtl, force);
-#ifdef CACHE_METRICS_BREAKAT
breakat_cache_.insert(s_cache, pp, s_cache.size() * sizeof(char_type));
}
-#endif
- if (pp->first == -1)
+ if (pp.first == -1)
return false;
- s = s.substr(0, pp->first);
- x = pp->second;
-#ifndef CACHE_METRICS_BREAKAT
- delete pp;
-#endif
+ s = s.substr(0, pp.first);
+ x = pp.second;
return true;
}
diff --git a/src/frontends/qt4/GuiFontMetrics.h b/src/frontends/qt4/GuiFontMetrics.h
index 438ebb4..ffbbe20 100644
--- a/src/frontends/qt4/GuiFontMetrics.h
+++ b/src/frontends/qt4/GuiFontMetrics.h
@@ -14,30 +14,16 @@
#include "frontends/FontMetrics.h"
+#include "support/Cache.h"
#include "support/docstring.h"
+#include "support/shared_ptr.h"
#include <QFont>
#include <QFontMetrics>
#include <QHash>
#include <QTextLayout>
-// Declare which font metrics elements have to be cached
-
-#define CACHE_METRICS_WIDTH
-#define CACHE_METRICS_BREAKAT
-// Qt 5.x already has its own caching of QTextLayout objects
-#if (QT_VERSION < 0x050000)
-#define CACHE_METRICS_QTEXTLAYOUT
-#endif
-
-#if defined(CACHE_METRICS_WIDTH) || defined(CACHE_METRICS_BREAKAT) \
- || defined(CACHE_METRICS_QTEXTLAYOUT)
-#define CACHE_SOME_METRICS
-#endif
-
-#ifdef CACHE_SOME_METRICS
-#include <QCache>
-#endif
+#include <memory>
namespace lyx {
namespace frontend {
@@ -80,15 +66,14 @@ public:
int width(QString const & str) const;
/// Return a pointer to a cached QTextLayout object
- QTextLayout const *
+ shared_ptr<QTextLayout const>
getTextLayout(docstring const & s, bool const rtl,
- double const wordspacing) const;
+ double const wordspacing) const;
private:
- std::pair<int, int> *
- breakAt_helper(docstring const & s, int const x,
- bool const rtl, bool const force) const;
+ std::pair<int, int> breakAt_helper(docstring const & s, int const x,
+ bool const rtl, bool const force) const;
/// The font
QFont font_;
@@ -98,21 +83,12 @@ private:
/// Cache of char widths
mutable QHash<char_type, int> width_cache_;
-
-#ifdef CACHE_METRICS_WIDTH
/// Cache of string widths
- mutable QCache<docstring, int> strwidth_cache_;
-#endif
-
-#ifdef CACHE_METRICS_BREAKAT
+ mutable Cache<docstring, int> strwidth_cache_;
/// Cache for breakAt
- mutable QCache<docstring, std::pair<int, int> > breakat_cache_;
-#endif
-
-#ifdef CACHE_METRICS_QTEXTLAYOUT
- /// Cache for QTextLayout:s
- mutable QCache<docstring, QTextLayout> qtextlayout_cache_;
-#endif
+ mutable Cache<docstring, std::pair<int, int> > breakat_cache_;
+ /// Cache for QTextLayout
+ mutable Cache<docstring, shared_ptr<QTextLayout> > qtextlayout_cache_;
struct AscendDescend {
int ascent;
diff --git a/src/frontends/qt4/GuiPainter.cpp b/src/frontends/qt4/GuiPainter.cpp
index 8607454..f87c266 100644
--- a/src/frontends/qt4/GuiPainter.cpp
+++ b/src/frontends/qt4/GuiPainter.cpp
@@ -473,7 +473,8 @@ int GuiPainter::text(int x, int y, docstring const & s,
// don't use the pixmap cache
setQPainterPen(computeColor(f.realColor()));
if (dir != Auto) {
- QTextLayout const * ptl = fm.getTextLayout(s, dir == RtL, wordspacing);
+ shared_ptr<QTextLayout const> ptl =
+ fm.getTextLayout(s, dir == RtL, wordspacing);
ptl->draw(this, QPointF(x, y - fm.maxAscent()));
}
else {
diff --git a/src/support/Cache.h b/src/support/Cache.h
new file mode 100644
index 0000000..dc6c54f
--- /dev/null
+++ b/src/support/Cache.h
@@ -0,0 +1,88 @@
+// -*- C++ -*-
+/**
+ * \file Cache.h
+ * This file is part of LyX, the document processor.
+ * Licence details can be found in the file COPYING.
+ *
+ * \author Guillaume Munch
+ *
+ * Full author contact details are available in file CREDITS.
+ */
+
+#ifndef CACHE_H
+#define CACHE_H
+
+#include <QCache>
+
+#include <list>
+#ifdef LYX_USE_CXX11
+#include <type_traits>
+#endif
+
+namespace lyx {
+
+/**
+ * Cache<Key, T> implements a cache where objects are stored by copy.
+ *
+ * This is a wrapper for QCache. See the documentation of QCache:
+ * <https://doc.qt.io/qt-5/qcache.html>.
+ *
+ * It is especially useful for storing shared pointers. This turns QCache into a
+ * shared-ownership cache with no risks of dangling pointer. It is also useful
+ * for small copyable objects.
+ *
+ * Use this rather than QCache directly, to avoid naked pointers.
+ */
+template <class Key, class Val>
+class Cache : private QCache<Key, Val> {
+#if !(defined(__GNUC__) && (__GNUC__ == 4) && (__GNUC_MINOR__ == 6))
+ static_assert(std::is_copy_constructible<Val>::value,
+ "lyx::Cache only stores copyable objects!");
+ static_assert(std::is_default_constructible<Val>::value,
+ "lyx::Cache only stores default-constructible objects!");
+ using Q = QCache<Key, Val>;
+#else
+ typedef QCache<Key, Val> Q;
+#endif
+
+public:
+ ///
+ Cache(int max_cost = 100) : Q(max_cost) {}
+ ///
+#ifdef LYX_USE_CXX11
+ bool insert(Key const & key, Val object, int cost = 1)
+ {
+ return Q::insert(key, new Val(std::move(object)), cost);
+ }
+#else
+ bool insert(Key const & key, Val const & object, int cost = 1)
+ {
+ return Q::insert(key, new Val(object), cost);
+ }
+#endif
+ // Returns the default value (e.g. null pointer) before using the result. If
+ // this is not convenient for your type, check if it exists beforehand with
+ // Cache::contains.
+ Val object(Key const & key) const
+ {
+ if (Val * obj = Q::object(key))
+ return *obj;
+ return Val();
+ }
+ /// Everything from QCache except QCache::take.
+ using Q::clear;
+ using Q::contains;
+ using Q::count;
+ using Q::remove;
+ using Q::size;
+ bool empty() const { return Q::isEmpty(); }
+ std::list<Key> keys() { return Q::keys().toStdList(); }
+ int max_cost() const { return Q::maxCost(); }
+ void set_max_cost(int cost) { Q::setMaxCost(cost); }
+ int total_cost() const { return Q::totalCost(); }
+ Val operator[](Key const & key) const { return object(key); }
+};
+
+} // namespace lyx
+
+#endif
diff --git a/src/support/Makefile.am b/src/support/Makefile.am
index ed6af57..a1dc3c2 100644
--- a/src/support/Makefile.am
+++ b/src/support/Makefile.am
@@ -33,6 +33,7 @@ liblyxsupport_a_SOURCES = \
FileMonitor.cpp \
RandomAccessList.h \
bind.h \
+ Cache.h \
ConsoleApplication.cpp \
ConsoleApplication.h \
ConsoleApplicationPrivate.h \
--
1.7.9.5
