From: Konrad P. <kon...@po...> - 2005-05-17 10:38:11
|
I would like to describe cloning mechanism that I propose for all CDK classes that need it: 1. Each class that need to support cloning should implement CloneableObject interface with public Object clone() method. The reason are: - the classes with new cloning support will be marked in this way - Cloneable interface does not actually enforce class to have clone method - as default is used clone method from Object that throws CloneNotSupportedException and we do not want it (bug #1117791) - Deep cloning will be safe because we will be able to use clone method for objects that implement CloneableObject without any exception occuring 2. Each class should have a copy constructor, that means e.g. public Element(Element e) { super(e); symbol=e.getSymbol(); atomicNumber=e.getAtomicNumber(); } Where we copy all publically available properties of object into current object. For every object cloning method would be of the same form e.g. for Element: public Object clone() { return new Element(this); } That are all modifications that are needed. 3. The very important feature of cloning contructor is that EVERY class that want to be cloned should have it. It makes our life easier since at the beginning of copy constructor we use super() - a copy constructor of the parent which knows how to copy parts of the object that were inherited! So we have to do less work! 4. I already did some work to implement this cloning model, however problems araise for AtomContainer and all its childrens. The reason is, it already have got something that looks like copy constructor and is widely used. The pseudo copy constructor was introduced probably for convenience and looks like: public AtomContainer(AtomContainer container) { this(); this.add(container); } It actually forgets about atomParities. Please show me the way in which I could change the copy constructor. Konrad -- Konrad Piwowarczyk <kon...@po...> |
From: Egon W. <e.w...@sc...> - 2005-05-17 10:52:42
|
On Tuesday 17 May 2005 12:38 pm, Konrad Piwowarczyk wrote: > I would like to describe cloning mechanism that I propose for all CDK > classes that need it: > 1. > Each class that need to support cloning should implement CloneableObject > interface with public Object clone() method. > The reason are: > - the classes with new cloning support will be marked in this way > - Cloneable interface does not actually enforce class to have clone > method - as default is used clone method from Object that throws > CloneNotSupportedException and we do not want it (bug #1117791) > - Deep cloning will be safe because we will be able to use clone method > for objects that implement CloneableObject without any exception > occuring PMD complains that the current clone() implementations don't throw CloneNotSupportedException... And I've noted that the current implementations catch exception, without giving any notice what so ever... Do you agree that it is a good idea to not use try/catch in these methods but have them throw CloneNotSupportedException's when exception occur? > 2. > Each class should have a copy constructor, that means e.g. > public Element(Element e) > { > super(e); > symbol=e.getSymbol(); > atomicNumber=e.getAtomicNumber(); > } > > Where we copy all publically available properties of object into current > object. > > For every object cloning method would be of the same form e.g. for > Element: > public Object clone() > { > return new Element(this); > } > > That are all modifications that are needed. See comment on CloneNotSupportedMethod... For example, what happens with some getProperty() that does not support cloning? > 3. > The very important feature of cloning contructor is that EVERY class > that want to be cloned should have it. It makes our life easier since at > the beginning of copy constructor we use super() - a copy constructor of > the parent which knows how to copy parts of the object that were > inherited! So we have to do less work! > > 4. > I already did some work to implement this cloning model, however > problems araise for AtomContainer and all its childrens. > The reason is, it already have got something that looks like copy > constructor and is widely used. > The pseudo copy constructor was introduced probably for convenience and > looks like: > public AtomContainer(AtomContainer container) > { > this(); > this.add(container); > } > It actually forgets about atomParities. That's a bug... I think the add(AtomContainer) method whould copy the atomParities too... > Please show me the way in which I could change the copy constructor. Looks good. I would propose to do this in two steps. One commit for the new constructors, which are useful in itself, and a second commit for the clone methods. Egon |
From: Konrad P. <kon...@po...> - 2005-05-17 11:21:15
|
Dnia 17-05-2005, wto o godzinie 12:52 +0200, Egon Willighagen napisa=B3(a): > PMD complains that the current clone() implementations don't throw=20 > CloneNotSupportedException... >=20 > And I've noted that the current implementations catch exception, withou= t=20 > giving any notice what so ever...=20 >=20 > Do you agree that it is a good idea to not use try/catch in these metho= ds but=20 > have them throw CloneNotSupportedException's when exception occur? Alright, CloneNotSupportedException should be thrown whenever something wrong happens while cloning. However I would like to have CloneableObject at least for marking changed classes. After all modifications it could be removed if not necessary. >=20 > For example, what happens with some getProperty() that does not support= =20 > cloning? Every object that support cloning should implement CloneableObject if not it is just assigned, not copied. > > 4. > > I already did some work to implement this cloning model, however > > problems araise for AtomContainer and all its childrens. > > The reason is, it already have got something that looks like copy > > constructor and is widely used. > > The pseudo copy constructor was introduced probably for convenience a= nd > > looks like: > > public AtomContainer(AtomContainer container) > > { > > this(); > > this.add(container); > > } > > It actually forgets about atomParities. >=20 > That's a bug... I think the add(AtomContainer) method whould copy the=20 > atomParities too... Should I modify it? > > Please show me the way in which I could change the copy constructor. >=20 > Looks good. >=20 > I would propose to do this in two steps. One commit for the new constru= ctors,=20 > which are useful in itself, and a second commit for the clone methods. >=20 > Egon I agree but I will try to split the procedure into (AtomContainer and descendands) and other objects Konrad=20 --=20 Konrad Piwowarczyk <kon...@po...> |
From: Konrad P. <kon...@po...> - 2005-05-17 11:30:22
|
Dnia 17-05-2005, wto o godzinie 12:52 +0200, Egon Willighagen napisa=B3(a): > For example, what happens with some getProperty() that does not support= =20 > cloning? > Egon At IRC channel Egon and I discussed the problem, here are our thoughts: jakt: The default behaviour for objects that do not provide Cloning colud be assigning it, Could you give some examples what can be placed into Properies? What problems can we face here? egonw: any object can be placed there and assigning sounds like a bad idea because when these properties are used, thye are likely to be changed and this would contradict the deep clone concept associated with clone()... jakt: The other behavior is abandone non cloneable objects but will produced object be consistent then? jakt: We could enforce object put into Properties to be Cloneable but we need then to catch CloneableNotSupportedEsception, and what to do then? The problem seems to be difficult Konrad --=20 Konrad Piwowarczyk <kon...@po...> |