Pimpl copying

[Peter Kümmel]

Peter Kümmel wrote:
> 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:

I've solved the issue by this patch:

SVN commit 562284 by kuemmel:

macro for pimpl handling:
 * consistent naming of the private implementation
 * explicit copying, checked by the linker
 * disable copying, checked at compile time
 * const pimpl


 M  +1 -0      TODO
 M  +2 -2      kdecore/ktempdir.cpp
 M  +1 -2      kdecore/ktempdir.h
 M  +2 -2      kdecore/tests/ktempdirtest.cpp
 M  +62 -0     kdemacros.h.cmake


--- trunk/KDE/kdelibs/TODO #562283:562284
@@ -10,6 +10,7 @@
   help prevent mistakes where developers forget to delete the pointer. Maybe
make use of
   Qt4 helper macros?
   (Frerich)
+  Use KDE_PIMPL_EN(DIS)ABLE_COPY to avoid compiler generated copy functions (Kümmel)

 - Move all utility functions away from KApplication. TBD: Make KApplication
   a very thin wrapper around QApplication. Ideally, KApplication should go
--- trunk/KDE/kdelibs/kdecore/ktempdir.cpp #562283:562284
@@ -53,7 +53,7 @@
 #include <kdebug.h>
 #include "kde_file.h"

-class KTempDir::Private
+class KTempDir::KTempDirPrivate
 {
 public:
    int _error;
@@ -66,7 +66,7 @@
 #define bAutoDelete d->_autoDelete
 };

-KTempDir::KTempDir(QString directoryPrefix, int mode) : d(new Private)
+KTempDir::KTempDir(QString directoryPrefix, int mode) : d(new KTempDirPrivate)
 {
    bAutoDelete = false;
    bExists = false;
--- trunk/KDE/kdelibs/kdecore/ktempdir.h #562283:562284
@@ -161,8 +161,7 @@
    void setError(int error);

 private:
-   class Private;
-   Private *const d;
+   KDE_PIMPL_DISABLE_COPY(KTempDir);
 };

 #endif
--- trunk/KDE/kdelibs/kdecore/tests/ktempdirtest.cpp #562283:562284
@@ -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());
@@ -51,7 +51,7 @@

 void KTempDirTest::testCreateSubDir()
 {
-	KTempDir dir = KTempDir("test");
+	KTempDir dir("test");
 	dir.setAutoDelete(true);
 	QVERIFY(dir.status() == 0);
 	QVERIFY(dir.exists());
--- trunk/KDE/kdelibs/kdemacros.h.cmake #562283:562284
@@ -361,4 +361,66 @@
 # define KDE_DUMMY_QHASH_FUNCTION(C)
 #endif

+/**
+ * @def KDE_PIMPL_ENABLE_COPY
+ *
+ * KDE_PIMPL_ENABLE_COPY is a macro for the
+ * consistent Pimpl (Pointer to Implementation) usage.
+ *
+ * It ensures that the code shows explicit how
+ * assignment and copying is handled.
+ *
+ * Using KDE_PIMPL_ENABLE_COPY one must also implement
+ * the copy constructor and the assignment operator.
+ *
+ * Using KDE_PIMPL_DISABLE_COPY makes the class NOT copyable
+ * by declaring the related functions as private.
+ *
+ * Example:
+ *
+ * \code
+ * class KClass {
+ * public:
+ *     KClass();
+ *     ~KClass();
+ * private:
+ *     KDE_PIMPL_ENABLE_COPY(KClass);
+ * };
+ *
+ * class KClass::KClassPrivate {
+ * public:
+ *     int data;
+ * };
+ *
+ * KClass::KClass() : d(new KClassPrivate) {}
+ * KClass::~KClass() { delete d; }
+ * KClass::KClass(const KClass& rhs) : d(new KClassPrivate) { operator=(rhs);}
+ * KClass& KClass::operator=(const KClass& rhs) {
+ *   if(this!=&rhs)
+ *       d->data = rhs.d->data;
+ *   return *this;
+ * }
+ * \endcode
+ *
+ */
+
+ /**
+ * @def KDE_PIMPL_DISABLE_COPY
+ *
+ * see:
+ * @sa KDE_PIMPL_ENABLE_COPY
+ */
+
+#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_ENABLE_COPY(X)  KDE_PIMPL_BASE_MACRO(public, X)
+#define KDE_PIMPL_DISABLE_COPY(X) KDE_PIMPL_BASE_MACRO(private,X)
+
+
 #endif /* _KDE_MACROS_H_ */