cxbrooks / test

Second test for bugzilla to git
0 stars 0 forks source link

Fix clone() for PropertyLattice #201

Closed cxbrooks closed 13 years ago

cxbrooks commented 14 years ago

Note: the issue was created automatically with bugzilla2github tool

Original bug ID: BZ#319 From: Elizabeth Latronico <beth@berkeley.edu> Reported version: 7.1.devel CC: pt-dev@chess.eecs.berkeley.edu

cxbrooks commented 14 years ago

+) This will probably have interact with fixing equals() for PropertyLattice (see additional bug report).

+) The intent seems to be that a specific PropertyLattice should be a singleton. What is a better way of implementing this?

+) What is the interaction / what are possible issues regarding user-defined custom lattices?

+) (Edward) The clone() method returns this. This is only correct if the object and all instances of subclasses are immutable. This seems extremely unlikely in a class that has an isConstant() method. Comment (Thomas M.): As far as I remember cloning was an issue when we combined the property system with model transformation. Jackie got it to run somehow, but unfortunately I don't remember the details. It's definitely something we did not understand in full detail, so some refactoring may be good. In my opinion the refactored code needs to be tested along with the model transformation.

+) Fix issues identified in FindBugs: http://chess.eecs.berkeley.edu/ptexternal/nightly/findbugs.htm

(Christopher) About Clonable, the FindBugs warning is: CN: In class ptolemy.data.properties.lattice.LatticeProperty In class ptolemy.data.properties.lattice.LatticeProperty In method ptolemy.data.properties.lattice.LatticeProperty.clone() At LatticeProperty.java:[line 74]

http://findbugs.sourceforge.net/bugDescriptions.html#CN_IMPLEMENTS_CLONE_BUT_NOT_CLONEABLE

says

"This class defines a clone() method but the class doesn't implement Cloneable. There are some situations in which this is OK (e.g., you want to control how subclasses can clone themselves), but just make sure that this is what you intended."

I made LatticeProperty implement Cloneable.

However, the LatticeProperty.clone returns itself:

/**
  * Return this LatticeProperty.
  * @ return The clone.
  * @ exception CloneNotSupportedException Not thrown in this base class.
  */
 public Object clone() throws CloneNotSupportedException {
     return this;
 }

Is this really what we want? http://findbugs.sourceforge.net/bugDescriptions.html#CN_IDIOM_NO_SUPER_CALL says:

"CN: clone method does not call super.clone() (CN_IDIOM_NO_SUPER_CALL) This non-final class defines a clone() method that does not call super.clone(). If this class ("A") is extended by a subclass ("B"), and the subclass B calls super.clone(), then it is likely that B's clone() method will return an object of type A, which violates the standard contract for clone().

If all clone() methods call super.clone(), then they are guaranteed to use Object.clone(), which always returns an object of the correct type."

I think that maybe LatticeProperty.clone should actually clone itself and not just return itself.

cxbrooks commented 13 years ago

ptolemy.data.properties was removed