[Uml-devel] removed clone() and copyInto()

[Gopala Krishna]

Hi,

 Thanks for the patch Eike. I have some thoughts on the patch.

> I tested the nice patch from Eike. I only added a parent initialization
> in the copy constructor due to a compiler warning:
> /**
>  * Copy constructor.
>  */
> UMLObject::UMLObject(const UMLObject &u)
>   : QObject(u.parent())
> {
>
> I haven't seen any problem.
>
> But I remember that there is some Qt macro Q_DISABLE_COPY(class)
> and after "googeling" I found very quickly
> http://lists.trolltech.com/qt-interest/2006-01/msg00621.html
> What shall we do with the patch? Do we need the clone() and copyInto()
> methods?
> Please, give some advice, feedback, experience. Shall we post a question
> on kde-devel?
>
Here is my feedback.
The patch isn't appropriate and introduces a crash.
To reproduce.

Create class a
Add operation void hello(bool param)
Now right click on hello in the list view -> Select properties
In the dialog box select param, click properties button
A new sub dialog pops in.
  However this sub dialog now fails to fill the fields with current value.
  Modifying and applying any attribute (name, type etc) crashes!

I haven't run it through gdb, but the following section in patch looks
very dangerous.

Index: dialogs/umloperationdialog.cpp
===================================================================
--- dialogs/umloperationdialog.cpp	(Revision 1126393)
+++ dialogs/umloperationdialog.cpp	(Arbeitskopie)
@@ -328,7 +328,7 @@

 void UMLOperationDialog::slotParameterProperties()
 {
-    UMLAttribute* pAtt = 0, * pOldAtt = 0;
+    UMLAttribute* pOldAtt = 0;

[snip]

........................................................
+        delete pOldAtt;
+        pOldAtt = tempAttribute;
........................................................
         if ( namingConflict ) {
             pOldAtt->setName( oldAttName ); // reset the name if
there was a naming conflict
         }
[snip]

The highligted part is the faulty part.
* It deletes the actual UMLAttribute (old one) which is stored in
UMLOperation which is further stored in UMLClassifier.
* And then assigns the temp attribute to just *local* variable.

This way the pointers in UMLOperation isn't rightly updated - infact
they have been invalidated without UMLOperation knowing that.

One of the fix would be to not delete the old object, but copy the new
object attribs to old one and delete the new one instead - i.e to
retain copyInto() method.

Now coming to replacing clone() method with copy constructor:
Qt discourages the usage of copy constructors for QObject subclasses
as indicated in its documentation.

http://doc.qt.nokia.com/4.7-snapshot/qobject.html#Q_DISABLE_COPY

Also conceptually copy-constructor gives the meaning of constructing
an exact replica of an object - i.e all the attributes (including that
of base class) are copied exactly into the newly constructed object.

However IIRC, in our case, two UMLObject's can't be identical i.e they
can't share same id for sure atleast. Also UMLObject is technically a
QObject, QObject isn't copyable and so any UMLObject too should not be
copyable.

As such, I  humbly request to retain the current clone() and
copyInto() methods :)

-- 
Cheers,
Gopala Krishna A