Copying removes composites from parent

[anhi]

Reported by Marcel on 17 Feb 39834598 13:20 UTC If some composite (Atom, Molecule, etc.) is copied, it is automatically removed from its parent.

Here is a tiny example:

    Molecule mol;
    Atom* atom1 = new Atom;
    mol.insert(*atom1);

    Atom atom2;
    *atom1 = atom2; // copy atom
    cout<<"molecule has "<<mol.countAtoms()<<" atoms"<<endl;


    System sys;
    sys.insert(mol);

    Molecule mol2;
    mol=mol2;  // copy molecule
    cout<<"system has "<<sys.countMolecules()<<" molecules"<<endl;

Output for this example will tell you that 'mol' has zero atoms and 'sys' has zero molecules.

Does anyone know if this was really ever intended?! And if yes, what should this behavior be good for?? ;-) Normally a copy operator should not interfere with the object's parent (but only copy the 'content' of the atom/molecule etc.), so that the user will definitly not expect the strange effect shown here!

[anhi]

Commented by dstoeckel on 10 Sep 39848887 15:33 UTC I think part of the behavior is because insert actually takes a reference and then gets the pointer to this reference. So no copying happens. If you now replace atom1 by atom2 the internal pointer still references the data in atom1. In order to prevent bad things from happening (e.g. corruption of the composite tree) atom1 removes itself from the child list and, of course, the copied atom2 does not re-register.

In its own crude and obscure way this behavior could really be correct... What is truly unfortunate is the design of the API. If ownership of the object is transfered, insert should really take a pointer instead of a reference. The STL way would be to use a copy, but I do not think we would want that here.

Also I am deferring this to 1.4. As there is no way we can fix this without changing the API and the semantics of insert.

[anhi]

Commented by Marcel on 27 Oct 39848912 21:20 UTC Replying to dstoeckel:

I think part of the behavior is because insert actually takes a reference and then gets the pointer to this reference.

Yes, that's right... that is also something that should never happen: Specifing a reference and then taking the address of this reference... this is really bad...

But that is not really the point here. The content of an atom is copied by

#cpp
*atom1 = atom2

[anhi]

Commented by Marcel on 7 Jun 39848918 12:00 UTC ups... that message was not yet read, sorry ;-)

... so, since the content of the atom is copied, not a pointer to an atom, I do not see why the atom really has to de-register itself from its parent!? I.e. how such this possibly lead to errors? That is something that will really never be expected by the user ;-)

[anhi]

Commented by dstoeckel on 25 Aug 39848972 17:46 UTC One problem I see would be the handling of bonds. Should they be copied or should the existing bonds be modified? Both ways of handling this will probably lead to problems (i.e. being leading to surprising behavior) :-(

Another thing is the parent pointer of the atom. Should it be copied over (which probably is done right now and is most likely the reason why you need to deregister.) or should it be kept from the old instance?

We could settle for only copying things like element, etc. which would would be reasonable and useful imho. However we do not know what code relies on the current behavior such that things can break virtually everywhere. This means we really need to be careful about this one... I think it is really non trivial to define which semantics are both: correct and sensible.

(BALL tends to go for correct but not always sensible semantics :D)

[anhi]

Commented by akdehof on 7 Jan 40936074 11:44 UTC Deferred to 1.4.1

[anhi]

Commented by akdehof on 15 May 40940482 04:58 UTC The best option seems to be to make the operator = () private... This will break compilation of potentially large amounts of code, so we should do this in a release that is not backward compatible anyhow... (BALL 2.0?)