- improved shared pointer behavior, still not there yet

Fri, 16 May 2014 01:22:24 +0300

author
Santeri Piippo <crimsondusk64@gmail.com>
date
Fri, 16 May 2014 01:22:24 +0300
changeset 769
8bb3bed44570
parent 768
29e6c5552388
child 770
b04c1e6ca1fb

- improved shared pointer behavior, still not there yet

src/actionsEdit.cc file | annotate | diff | comparison | revisions
src/addObjectDialog.cc file | annotate | diff | comparison | revisions
src/ldDocument.cc file | annotate | diff | comparison | revisions
src/ldObject.cc file | annotate | diff | comparison | revisions
src/ldObject.h file | annotate | diff | comparison | revisions
src/main.cc file | annotate | diff | comparison | revisions
--- a/src/actionsEdit.cc	Fri May 09 12:06:56 2014 +0300
+++ b/src/actionsEdit.cc	Fri May 16 01:22:24 2014 +0300
@@ -177,7 +177,6 @@
 		// after the first one.
 		getCurrentDocument()->setObject (index, triangles[0]);
 		getCurrentDocument()->insertObj (index + 1, triangles[1]);
-		obj->destroy();
 		num++;
 	}
 
--- a/src/addObjectDialog.cc	Fri May 09 12:06:56 2014 +0300
+++ b/src/addObjectDialog.cc	Fri May 16 01:22:24 2014 +0300
@@ -265,9 +265,7 @@
 	layout->addWidget (bbx_buttons, 5, 0, 1, 4);
 	setLayout (layout);
 	setWindowTitle (format (tr ("Edit %1"), typeName));
-
 	setWindowIcon (icon);
-	defaults->destroy();
 }
 
 // =============================================================================
--- a/src/ldDocument.cc	Fri May 09 12:06:56 2014 +0300
+++ b/src/ldDocument.cc	Fri May 16 01:22:24 2014 +0300
@@ -379,7 +379,6 @@
 			line.chop (1);
 
 		LDObjectPtr obj = parseLine (line);
-		fprint (stderr, "got obj: %1\n", obj.data());
 
 		// Check for parse errors and warn about tthem
 		if (obj->type() == LDObject::EError)
--- a/src/ldObject.cc	Fri May 09 12:06:56 2014 +0300
+++ b/src/ldObject.cc	Fri May 16 01:22:24 2014 +0300
@@ -32,7 +32,10 @@
 CFGENTRY (Int, defaultLicense, 0);
 
 // List of all LDObjects
-static LDObjectWeakList g_LDObjects;
+static QMap<long, LDObjectWeakPtr> g_allObjects;
+
+// Temporary resource
+LDObjectPtr g_temporaryLDObject;
 
 // =============================================================================
 // LDObject constructors
@@ -40,13 +43,20 @@
 LDObject::LDObject() :
 	m_isHidden (false),
 	m_isSelected (false),
+	m_isDestructed (false),
 	m_document (null),
 	qObjListEntry (null)
 {
+	LDObjectPtr selfptr (this, [](LDObject* obj){ obj->finalDelete(); });
 	memset (m_coords, 0, sizeof m_coords);
+	m_self = selfptr;
 	chooseID();
-	g_LDObjects << LDObjectWeakPtr (thisptr());
+	g_allObjects[id()] = self();
 	setRandomColor (QColor::fromHsv (rand() % 360, rand() % 256, rand() % 96 + 128));
+
+	// This prevents the object from being prematurely deleted just because
+	// selfptr falls out of scope before it's caught by spawn()
+	g_temporaryLDObject = selfptr;
 }
 
 // =============================================================================
@@ -55,13 +65,18 @@
 {
 	int32 id = 1; // 0 shalt be null
 
-	for (LDObjectWeakPtr obj : g_LDObjects)
+	for (auto it = g_allObjects.begin(); it != g_allObjects.end(); ++it)
 	{
-		LDObjectPtr obj2 = obj.toStrongRef();
-		assert (obj2 != this);
+		LDObjectPtr obj = it->toStrongRef();
+
+		if (not obj)
+			fprint (stdout, "obj #%1 wasn't removed properly\n", it.key());
 
-		if (obj2->id() >= id)
-			id = obj2->id() + 1;
+		assert (obj != this);
+		assert (obj != null);
+
+		if (obj->id() >= id)
+			id = obj->id() + 1;
 	}
 
 	setID (id);
@@ -226,7 +241,7 @@
 void LDObject::swap (LDObjectPtr other)
 {
 	assert (document() == other->document());
-	document()->swapObjects (thisptr(), other);
+	document()->swapObjects (self(), other);
 }
 
 // =============================================================================
@@ -263,10 +278,6 @@
 
 // =============================================================================
 //
-LDSubfile::~LDSubfile() {}
-
-// =============================================================================
-//
 void LDObject::destroy()
 {
 	// If this object was selected, unselect it now
@@ -275,13 +286,24 @@
 
 	// If this object was associated to a file, remove it off it now
 	if (document())
-		document()->forgetObject (thisptr());
+		document()->forgetObject (self());
 
 	// Delete the GL lists
-	g_win->R()->forgetObject (thisptr());
+	g_win->R()->forgetObject (self());
 
 	// Remove this object from the list of LDObjects
-	g_LDObjects.removeOne (LDObjectWeakPtr (thisptr()));
+	g_allObjects.erase (g_allObjects.find (id()));
+	setDestructed (true);
+}
+
+//
+// Deletes the object. Only the shared pointer is to call this!
+//
+void LDObject::finalDelete()
+{
+	if (not isDestructed())
+		destroy();
+
 	delete this;
 }
 
@@ -334,7 +356,7 @@
 	for (LDObjectPtr obj : objs)
 	{
 		// Set the parent now so we know what inlined the object.
-		obj->setParent (LDObjectWeakPtr (thisptr()));
+		obj->setParent (self());
 		transformObject (obj, transform(), position(), color());
 	}
 
@@ -440,10 +462,7 @@
 //
 String LDObject::typeName (LDObject::Type type)
 {
-	LDObjectPtr obj = LDObject::getDefault (type);
-	String name = obj->typeName();
-	obj->destroy();
-	return name;
+	return LDObject::getDefault (type)->typeName();
 }
 
 // =============================================================================
@@ -486,9 +505,9 @@
 LDObjectPtr LDObject::topLevelParent()
 {
 	if (parent() == null)
-		return thisptr();
+		return self();
 
-	LDObjectWeakPtr it (thisptr());
+	LDObjectWeakPtr it (self());
 
 	while (it.toStrongRef()->parent() != null)
 		it = it.toStrongRef()->parent();
@@ -528,13 +547,13 @@
 {
 	if (hasMatrix())
 	{
-		LDMatrixObjectPtr mo = thisptr().dynamicCast<LDMatrixObject>();
+		LDMatrixObjectPtr mo = self().toStrongRef().dynamicCast<LDMatrixObject>();
 		mo->setPosition (mo->position() + vect);
 	}
 	elif (type() == LDObject::EVertex)
 	{
 		// ugh
-		thisptr().staticCast<LDVertex>()->pos += vect;
+		self().toStrongRef().staticCast<LDVertex>()->pos += vect;
 	}
 	else
 	{
@@ -664,13 +683,10 @@
 //
 LDObjectPtr LDObject::fromID (int id)
 {
-	for (LDObjectWeakPtr obj : g_LDObjects)
-	{
-		LDObjectPtr obj2 = obj.toStrongRef();
+	auto it = g_allObjects.find (id);
 
-		if (obj2->id() == id)
-			return obj;
-	}
+	if (it != g_allObjects.end())
+		return *it;
 
 	return LDObjectPtr();
 }
@@ -719,7 +735,7 @@
 //
 void LDObject::setColor (const int& val)
 {
-	changeProperty (thisptr(), &m_color, val);
+	changeProperty (self(), &m_color, val);
 }
 
 // =============================================================================
@@ -736,33 +752,37 @@
 	if (document() != null)
 		document()->vertexChanged (*m_coords[i], vert);
 
-	changeProperty (thisptr(), &m_coords[i], LDSharedVertex::getSharedVertex (vert));
+	changeProperty (self(), &m_coords[i], LDSharedVertex::getSharedVertex (vert));
 }
 
 // =============================================================================
 //
 void LDMatrixObject::setPosition (const Vertex& a)
 {
-	if (linkPointer()->document() != null)
-		linkPointer()->document()->removeKnownVerticesOf (linkPointer());
+	LDObjectPtr ref = linkPointer().toStrongRef();
+
+	if (ref->document() != null)
+		ref->document()->removeKnownVerticesOf (ref);
 
-	changeProperty (linkPointer(), &m_position, LDSharedVertex::getSharedVertex (a));
+	changeProperty (ref, &m_position, LDSharedVertex::getSharedVertex (a));
 
-	if (linkPointer()->document() != null)
-		linkPointer()->document()->addKnownVerticesOf (linkPointer());
+	if (ref->document() != null)
+		ref->document()->addKnownVerticesOf (ref);
 }
 
 // =============================================================================
 //
 void LDMatrixObject::setTransform (const Matrix& val)
 {
-	if (linkPointer()->document() != null)
-		linkPointer()->document()->removeKnownVerticesOf (linkPointer());
+	LDObjectPtr ref = linkPointer().toStrongRef();
+
+	if (ref->document() != null)
+		ref->document()->removeKnownVerticesOf (ref);
 
-	changeProperty (linkPointer(), &m_transform, val);
+	changeProperty (ref, &m_transform, val);
 
-	if (linkPointer()->document() != null)
-		linkPointer()->document()->addKnownVerticesOf (linkPointer());
+	if (ref->document() != null)
+		ref->document()->addKnownVerticesOf (ref);
 }
 
 // =============================================================================
@@ -808,7 +828,7 @@
 void LDObject::select()
 {
 	assert (document() != null);
-	document()->addToSelection (thisptr());
+	document()->addToSelection (self());
 }
 
 // =============================================================================
@@ -816,7 +836,7 @@
 void LDObject::deselect()
 {
 	assert (document() != null);
-	document()->removeFromSelection (thisptr());
+	document()->removeFromSelection (self());
 }
 
 // =============================================================================
@@ -852,7 +872,7 @@
 void LDSubfile::setFileInfo (const LDDocumentPointer& a)
 {
 	if (document() != null)
-		document()->removeKnownVerticesOf (thisptr());
+		document()->removeKnownVerticesOf (self());
 
 	m_fileInfo = a;
 
@@ -867,5 +887,5 @@
 	}
 
 	if (document() != null)
-		document()->addKnownVerticesOf (thisptr());
+		document()->addKnownVerticesOf (self());
 };
--- a/src/ldObject.h	Fri May 09 12:06:56 2014 +0300
+++ b/src/ldObject.h	Fri May 16 01:22:24 2014 +0300
@@ -31,6 +31,7 @@
 	}															\
 	virtual String asText() const override;						\
 	virtual void invert() override;								\
+																\
 protected:														\
 	friend class QSharedPointer<LD##T>::ExternalRefCount;		\
 
@@ -65,11 +66,13 @@
 {
 	PROPERTY (public,		bool,				isHidden,		setHidden,		STOCK_WRITE)
 	PROPERTY (public,		bool,				isSelected,		setSelected,	STOCK_WRITE)
+	PROPERTY (public,		bool,				isDestructed,	setDestructed,	STOCK_WRITE)
 	PROPERTY (public,		LDObjectWeakPtr,	parent,			setParent,		STOCK_WRITE)
 	PROPERTY (public,		LDDocument*,		document,		setDocument,	STOCK_WRITE)
 	PROPERTY (private,		int,				id,				setID,			STOCK_WRITE)
 	PROPERTY (public,		int,				color,			setColor,		CUSTOM_WRITE)
 	PROPERTY (private,		QColor,				randomColor,	setRandomColor,	STOCK_WRITE)
+	PROPERTY (private,		LDObjectWeakPtr,	self,			setSelf,		STOCK_WRITE)
 
 	public:
 		// Object type codes.
@@ -177,8 +180,12 @@
 		// TODO: make this private!
 		QListWidgetItem* qObjListEntry;
 
+		// This is public because I cannot protect it as the lambda deletor would
+		// have to be the friend. Do not call this! Ever!
+		void finalDelete();
+
 	protected:
-		// LDObjects are to be deleted with the deleteSelf() method, not with
+		// LDObjects are to be deleted with the finalDelete() method, not with
 		// operator delete. This is because it seems virtual functions cannot
 		// be properly called from the destructor, thus a normal method must
 		// be used instead. The destructor also doesn't seem to be able to
@@ -191,26 +198,23 @@
 		// pointer's base class still calls operator delete directly in one of
 		// its methods. The method should never be called but we need to declare
 		// the class making this delete call a friend anyway.
+		friend class QSharedPointer<LDObject>;
 		friend class QSharedPointer<LDObject>::ExternalRefCount;
 
-		inline LDObjectPtr thisptr()
-		{
-			return LDObjectPtr (this);
-		}
-
 	private:
 		LDSharedVertex*	m_coords[4];
 };
 
 //
 // Makes a new LDObject. This makes the shared pointer always use the custom
-// deleter so that all deletions go through destroy();
+// deleter so that all deletions go through finalDelete();
 //
 template<typename T, typename... Args>
 inline QSharedPointer<T> spawn (Args... args)
 {
 	static_assert (std::is_base_of<LDObject, T>::value, "spawn may only be used with LDObject-derivatives");
-	return QSharedPointer<T> (new T (args...), [](T* obj){ obj->destroy(); });
+	LDObject* obj = new T (args...);
+	return obj->self().toStrongRef().staticCast<T>();
 }
 
 NUMERIC_ENUM_OPERATORS (LDObject::Type)
@@ -271,8 +275,8 @@
 //
 class LDMatrixObject
 {
-	PROPERTY (public,	LDObjectPtr,	linkPointer,	setLinkPointer,	STOCK_WRITE)
-	PROPERTY (public,	Matrix,		transform,		setTransform,	CUSTOM_WRITE)
+	PROPERTY (public,	LDObjectWeakPtr,	linkPointer,	setLinkPointer,	STOCK_WRITE)
+	PROPERTY (public,	Matrix,				transform,		setTransform,	CUSTOM_WRITE)
 
 	public:
 		LDMatrixObject() :
@@ -446,18 +450,15 @@
 
 		Q_DECLARE_FLAGS (InlineFlags, InlineFlag)
 
-		LDSubfile()
+		LDSubfile() :
+			LDObject()
 		{
-			setLinkPointer (QSharedPointer<LDSubfile> (this));
+			setLinkPointer (self());
 		}
 
-		// Inlines this subfile. Note that return type is an array of heap-allocated
-		// LDObject copies, they must be deleted manually.
+		// Inlines this subfile.
 		LDObjectList inlineContents (bool deep, bool render);
 		QList<LDPolygon> inlinePolygons();
-
-	protected:
-		~LDSubfile();
 };
 
 Q_DECLARE_OPERATORS_FOR_FLAGS (LDSubfile::InlineFlags)
--- a/src/main.cc	Fri May 09 12:06:56 2014 +0300
+++ b/src/main.cc	Fri May 16 01:22:24 2014 +0300
@@ -43,12 +43,6 @@
 
 CFGENTRY (Bool, firstStart, true);
 
-static void deleteline (LDLine* obj)
-{
-	fprintf (stderr, "delete %p\n", obj);
-	delete obj;
-}
-
 // =============================================================================
 //
 int main (int argc, char* argv[])
@@ -74,15 +68,7 @@
 	initColors();
 	MainWindow* win = new MainWindow;
 	newFile();
-	//win->show();
-
-	QSharedPointer<LDLine> obj (new LDLine, &deleteline);
-	fprint (stderr, "%1: %2\n", obj.data(), obj->type());
-	//class A { public: virtual ~A(){} virtual int foo() = 0; void destroy() { delete this; }};
-	//class B : public A { public: virtual ~B(){} virtual int foo() override { return 5; }};
-	//QSharedPointer<A> a (new B, [](A* p) { p->destroy(); });
-	//fprintf (stderr, "%p (%lu bytes)\n", a.data(), sizeof *a);
-	exit (0);
+	win->show();
 
 	// If this is the first start, get the user to configuration. Especially point
 	// them to the profile tab, it's the most important form to fill in.

mercurial