Pimpl copying

[Peter Kümmel]

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