Pimpl copying
Peter Kümmel
syntheticpp at gmx.net
Thu Jul 13 22:05:03 BST 2006
While debugging KTempDirTest on windows I've found that this patch
Index: ktempdirtest.cpp
===================================================================
--- ktempdirtest.cpp (revision 561767)
+++ ktempdirtest.cpp (working copy)
@@ -26,7 +26,7 @@
void KTempDirTest::testBasic()
{
- KTempDir dir = KTempDir("test");
+ KTempDir dir("test");
QVERIFY(dir.status() == 0);
QVERIFY(dir.exists());
QVERIFY(QDir(dir.name()).exists());
avoids a crash caused by dereferencing a uninitialized variable.
(wonder why it works with gcc)
The problem is that KTempDirTest uses the Pimpl idiom, but does
not define a copy ctor/operator, so all private data gets lost
on copying, nor does it forbid copying.
I fear kdelibs is full of this bug and think we should catch
such constructs at compile time.
Shouldn't we introduce a coding policy something like this:
Index: ktempdir.h
===================================================================
--- ktempdir.h (revision 561767)
+++ ktempdir.h (working copy)
@@ -25,6 +25,18 @@
class QString;
class QDir;
+
+#define KDE_PIMPL_BASE_MACRO(COPY_ACCESS,X) \
+COPY_ACCESS: \
+ X(const X##&); \
+ X##& operator=(const X##&); \
+private: \
+ class X##Private; \
+ X##Private* const d;
+
+#define KDE_PIMPL_COPYABLE(X) KDE_PIMPL_BASE_MACRO(public, X)
+#define KDE_PIMPL_UNCOPYABLE(X) KDE_PIMPL_BASE_MACRO(private,X)
+
/**
* The KTempDir class creates a unique directory for temporary use.
*
@@ -161,8 +173,7 @@
void setError(int error);
private:
- class Private;
- Private *const d;
+ KDE_PIMPL_COPYABLE(KTempDir);
};
#endif
Index: ktempdir.cpp
===================================================================
--- ktempdir.cpp (revision 561767)
+++ ktempdir.cpp (working copy)
@@ -53,7 +53,7 @@
#include <kdebug.h>
#include "kde_file.h"
-class KTempDir::Private
+class KTempDir::KTempDirPrivate
{
public:
int _error;
@@ -66,8 +66,22 @@
#define bAutoDelete d->_autoDelete
};
-KTempDir::KTempDir(QString directoryPrefix, int mode) : d(new Private)
+KTempDir::KTempDir(const KTempDir& rhs) : d(new KTempDirPrivate)
{
+ operator=(rhs);
+}
+
+KTempDir& KTempDir::operator=(const KTempDir& rhs)
+{
+ mError = rhs.d->_error;
+ mTmpName = rhs.d->_tmpName;
+ bExists = rhs.d->_exists;
+ bAutoDelete = rhs.d->_autoDelete;
+ return *this;
+}
+
+KTempDir::KTempDir(QString directoryPrefix, int mode) : d(new KTempDirPrivate)
+{
bAutoDelete = false;
bExists = false;
mError=0;
This introduces a new macro KDE_PIMPL_COPYABLE
which declares the pimpl AND the copy functions.
KDE_PIMPL_UNCOPYABLE declares the copy functions
as private, so we get an error at compile time.
Or is there already a Qt way to handle this?
Peter
More information about the kde-core-devel
mailing list