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.

src/frontends/qt4/GuiFontMetrics.cpp

@@ -27,14 +27,11 @@
 #define DISABLE_PMPROF
 #include "support/pmprof.h"
 
-#ifdef CACHE_SOME_METRICS
 #include <QByteArray>
-#endif
 
 using namespace std;
 using namespace lyx::support;
 
-#ifdef CACHE_SOME_METRICS
 namespace std {
 
 /*
@@ -49,11 +46,21 @@ uint qHash(lyx::docstring const & s)
 }
 
 }
-#endif
 
 namespace lyx {
 namespace frontend {
 
+
+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.
@@ -83,16 +90,10 @@ inline QChar const ucs4_to_qchar(char_type const ucs4)
  * Note that all these numbers are arbitrary.
  */
 GuiFontMetrics::GuiFontMetrics(QFont const & font)
-	: font_(font), metrics_(font, 0)
+	: font_(font), metrics_(font, 0),
-#ifdef CACHE_METRICS_WIDTH
+	  strwidth_cache_(cache_metrics_width_size),
-	, strwidth_cache_(1 << 19)
+	  breakat_cache_(cache_metrics_breakat_size),
-#endif
+	  qtextlayout_cache_(cache_metrics_qtextlayout_size)
-#ifdef CACHE_METRICS_BREAKAT
-	, breakat_cache_(1 << 19)
-#endif
-#ifdef CACHE_METRICS_QTEXTLAYOUT
-	,  qtextlayout_cache_(500)
-#endif
 {
 }
 
@@ -177,13 +178,10 @@ int GuiFontMetrics::rbearing(char_type c) const
 
 int GuiFontMetrics::width(docstring const & s) const
 {
-	PROFILE_THIS_BLOCK(width)
+	PROFILE_THIS_BLOCK(width);
-#ifdef CACHE_METRICS_WIDTH
+	if (strwidth_cache_.contains(s))
-	int * pw = strwidth_cache_[s];
+		return strwidth_cache_[s];
-	if (pw)
+	PROFILE_CACHE_MISS(width);
-		return *pw;
-	PROFILE_CACHE_MISS(width)
-#endif
 	/* 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
@@ -205,9 +203,7 @@ int GuiFontMetrics::width(docstring const & s) const
 		tl.endLayout();
 		w = int(line.naturalTextWidth());
 	}
-#ifdef CACHE_METRICS_WIDTH
+	strwidth_cache_.insert(s, w, s.size() * sizeof(char_type));
-	strwidth_cache_.insert(s, new int(w), s.size() * sizeof(char_type));
-#endif
 	return w;
 }
 
@@ -230,33 +226,28 @@ 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
 {
-	PROFILE_THIS_BLOCK(getTextLayout)
+	PROFILE_THIS_BLOCK(getTextLayout);
-	QTextLayout * ptl;
+	docstring const s_cache =
-#ifdef CACHE_METRICS_QTEXTLAYOUT
+		s + (rtl ? "r" : "l") + convert<docstring>(wordspacing);
-	docstring const s_cache = s + (rtl ? "r" : "l") + convert<docstring>(wordspacing);
+	if (auto ptl = qtextlayout_cache_[s_cache])
-	ptl = qtextlayout_cache_[s_cache];
+		return ptl;
-	if (!ptl) {
+	PROFILE_CACHE_MISS(getTextLayout);
-		PROFILE_CACHE_MISS(getTextLayout)
+	auto const ptl = make_shared<QTextLayout>();
-#endif
+	ptl->setCacheEnabled(true);
-		ptl = new QTextLayout();
+	ptl->setText(toqstr(s));
-		ptl->setCacheEnabled(true);
+	QFont copy = font_;
-		ptl->setText(toqstr(s));
+	copy.setWordSpacing(wordspacing);
-		QFont copy = font_;
+	ptl->setFont(copy);
-		copy.setWordSpacing(wordspacing);
+	// Note that both setFlags and the enums are undocumented
-		ptl->setFont(copy);
+	ptl->setFlags(rtl ? Qt::TextForceRightToLeft : Qt::TextForceLeftToRight);
-		// Note that both setFlags and the enums are undocumented
+	ptl->beginLayout();
-		ptl->setFlags(rtl ? Qt::TextForceRightToLeft : Qt::TextForceLeftToRight);
+	ptl->createLine();
-		ptl->beginLayout();
+	ptl->endLayout();
-		ptl->createLine();
+	qtextlayout_cache_.insert(s_cache, ptl);
-		ptl->endLayout();
-#ifdef CACHE_METRICS_QTEXTLAYOUT
-		qtextlayout_cache_.insert(s_cache, ptl);
-	}
-#endif
 	return ptl;
 }
 
@@ -266,7 +257,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).
@@ -279,7 +270,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));
@@ -328,7 +319,7 @@ int GuiFontMetrics::countExpanders(docstring const & str) const
 }
 
 
-pair<int, int> *
+pair<int, int>
 GuiFontMetrics::breakAt_helper(docstring const & s, int const x,
                                bool const rtl, bool const force) const
 {
@@ -373,7 +364,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 {-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).
@@ -398,35 +389,31 @@ 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 {len, int(line.naturalTextWidth())};
 }
 
 
 bool GuiFontMetrics::breakAt(docstring & s, int & x, bool const rtl, bool const force) const
 {
-	PROFILE_THIS_BLOCK(breakAt)
+	PROFILE_THIS_BLOCK(breakAt);
 	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];
+	docstring const s_cache =
-	if (!pp) {
+		s + convert<docstring>(x) + (rtl ? "r" : "l") + (force ? "f" : "w");
-		PROFILE_CACHE_MISS(breakAt)
+	pair<int, int> pp;
-#endif
+
+	if (breakat_cache_.contains(s_cache))
+		pp = breakat_cache_[s_cache];
+	else {
+		PROFILE_CACHE_MISS(breakAt);
 		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);
+	s = s.substr(0, pp.first);
-	x = pp->second;
+	x = pp.second;
-#ifndef CACHE_METRICS_BREAKAT
-	delete pp;
-#endif
 	return true;
 }
 

src/frontends/qt4/GuiFontMetrics.h

@@ -14,6 +14,7 @@
 
 #include "frontends/FontMetrics.h"
 
+#include "support/Cache.h"
 #include "support/docstring.h"
 
 #include <QFont>
@@ -21,23 +22,7 @@
 #include <QHash>
 #include <QTextLayout>
 
-// Declare which font metrics elements have to be cached
+#include <memory>
-
-#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
 
 namespace lyx {
 namespace frontend {
@@ -82,15 +67,14 @@ public:
 	int width(QString const & str) const;
 
 	/// Return a pointer to a cached QTextLayout object
-	QTextLayout const *
+	std::shared_ptr<QTextLayout const>
 	getTextLayout(docstring const & s, bool const rtl,
-                  double const wordspacing) const;
+	              double const wordspacing) const;
 
 private:
 
-	std::pair<int, int> *
+	std::pair<int, int> breakAt_helper(docstring const & s, int const x,
-	breakAt_helper(docstring const & s, int const x,
+	                                   bool const rtl, bool const force) const;
-	               bool const rtl, bool const force) const;
 
 	/// The font
 	QFont font_;
@@ -100,21 +84,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_;
+	mutable Cache<docstring, int> strwidth_cache_;
-#endif
-
-#ifdef CACHE_METRICS_BREAKAT
 	/// Cache for breakAt
-	mutable QCache<docstring, std::pair<int, int>> breakat_cache_;
+	mutable Cache<docstring, std::pair<int, int>> breakat_cache_;
-#endif
+	/// Cache for QTextLayout
-
+	mutable Cache<docstring, std::shared_ptr<QTextLayout>> qtextlayout_cache_;
-#ifdef CACHE_METRICS_QTEXTLAYOUT
-	/// Cache for QTextLayout:s
-	mutable QCache<docstring, QTextLayout> qtextlayout_cache_;
-#endif
 
 	struct AscendDescend {
 		int ascent;

src/frontends/qt4/GuiPainter.cpp

@@ -481,7 +481,8 @@ void 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 {

src/support/Cache.h

@@ -0,0 +1,75 @@
+// -*- 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>
+#include <type_traits>
+
+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> {
+	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>;
+
+public:
+	///
+	Cache(int max_cost = 100) : Q(max_cost) {}
+	///
+	bool insert(Key const & key, Val object, int cost = 1)
+	{
+		return Q::insert(key, new Val(std::move(object)), cost);
+	}
+	// 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

src/support/Makefile.am

@@ -35,6 +35,7 @@ liblyxsupport_a_SOURCES = \
 	FileMonitor.cpp \
 	RandomAccessList.h \
 	bind.h \
+	Cache.h \
 	Changer.h \
 	ConsoleApplication.cpp \
 	ConsoleApplication.h \