Clean-up of copy/clone methods in Ptg hierarchy

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@836343 13f79535-47bb-0310-9956-ffa450edef68

src/java/org/apache/poi/hssf/record/SharedFormulaRecord.java

@@ -113,14 +113,15 @@ public final class SharedFormulaRecord extends SharedValueRecordBase {
                 originalOperandClass = ptg.getPtgClass();
             }
             if (ptg instanceof RefPtgBase) {
-              RefPtgBase refNPtg = (RefPtgBase)ptg;
+                RefPtgBase refNPtg = (RefPtgBase)ptg;
-              ptg = new RefPtg(fixupRelativeRow(formulaRow,refNPtg.getRow(),refNPtg.isRowRelative()),
+                ptg = new RefPtg(fixupRelativeRow(formulaRow,refNPtg.getRow(),refNPtg.isRowRelative()),
                                      fixupRelativeColumn(formulaColumn,refNPtg.getColumn(),refNPtg.isColRelative()),
                                      refNPtg.isRowRelative(),
                                      refNPtg.isColRelative());
+                ptg.setClass(originalOperandClass);
             } else if (ptg instanceof AreaPtgBase) {
-              AreaPtgBase areaNPtg = (AreaPtgBase)ptg;
+                AreaPtgBase areaNPtg = (AreaPtgBase)ptg;
-              ptg = new AreaPtg(fixupRelativeRow(formulaRow,areaNPtg.getFirstRow(),areaNPtg.isFirstRowRelative()),
+                ptg = new AreaPtg(fixupRelativeRow(formulaRow,areaNPtg.getFirstRow(),areaNPtg.isFirstRowRelative()),
                                 fixupRelativeRow(formulaRow,areaNPtg.getLastRow(),areaNPtg.isLastRowRelative()),
                                 fixupRelativeColumn(formulaColumn,areaNPtg.getFirstColumn(),areaNPtg.isFirstColRelative()),
                                 fixupRelativeColumn(formulaColumn,areaNPtg.getLastColumn(),areaNPtg.isLastColRelative()),
@@ -128,15 +129,13 @@ public final class SharedFormulaRecord extends SharedValueRecordBase {
                                 areaNPtg.isLastRowRelative(),
                                 areaNPtg.isFirstColRelative(),
                                 areaNPtg.isLastColRelative());
-            } else {
-                if (false) {// do we need a ptg clone here?
-                    ptg = ptg.copy();
-                }
-            }
-            if (!ptg.isBaseToken()) {
                 ptg.setClass(originalOperandClass);
+            } else if (ptg instanceof OperandPtg<?>) {
+                // Any subclass of OperandPtg is mutable, so it's safest to not share these instances.
+                ptg = ((OperandPtg<?>) ptg).copy();
+            } else {
+            	// all other Ptgs are immutable and can be shared
             }
-
             newPtgStack[k] = ptg;
         }
         return newPtgStack;

src/java/org/apache/poi/hssf/record/TextObjectRecord.java

@@ -19,6 +19,7 @@ package org.apache.poi.hssf.record;
 
 import org.apache.poi.hssf.record.cont.ContinuableRecord;
 import org.apache.poi.hssf.record.cont.ContinuableRecordOutput;
+import org.apache.poi.hssf.record.formula.OperandPtg;
 import org.apache.poi.hssf.record.formula.Ptg;
 import org.apache.poi.hssf.usermodel.HSSFRichTextString;
 import org.apache.poi.util.BitField;
@@ -74,13 +75,14 @@ public final class TextObjectRecord extends ContinuableRecord {
 	 */
 	private int _unknownPreFormulaInt;
 	/** expect tRef, tRef3D, tArea, tArea3D or tName */
-	private Ptg _linkRefPtg;
+	private OperandPtg<?> _linkRefPtg;
 	/**
 	 * Not clear if needed .  Excel seems to be OK if this byte is not present.
 	 * Value is often the same as the earlier firstColumn byte. */
 	private Byte _unknownPostFormulaByte;
 
 	public TextObjectRecord() {
+		//
 	}
 
 	public TextObjectRecord(RecordInputStream in) {
@@ -106,7 +108,7 @@ public final class TextObjectRecord extends ContinuableRecord {
 				throw new RecordFormatException("Read " + ptgs.length
 						+ " tokens but expected exactly 1");
 			}
-			_linkRefPtg = ptgs[0];
+			_linkRefPtg = (OperandPtg<?>) ptgs[0];
 			if (in.remaining() > 0) {
 				_unknownPostFormulaByte = Byte.valueOf(in.readByte());
 			} else {

src/java/org/apache/poi/hssf/record/formula/Area3DPtg.java

@@ -32,7 +32,7 @@ import org.apache.poi.util.LittleEndianOutput;
  * @author avik
  * @author Jason Height (jheight at chariot dot net dot au)
  */
-public final class Area3DPtg extends AreaPtgBase implements WorkbookDependentFormula, ExternSheetReferenceToken {
+public final class Area3DPtg extends AreaPtgBase<Area3DPtg> implements WorkbookDependentFormula, ExternSheetReferenceToken {
 	public final static byte sid = 0x3b;
 	private final static int SIZE = 11; // 10 + 1 for Ptg
 

src/java/org/apache/poi/hssf/record/formula/AreaPtgBase.java

@@ -29,7 +29,7 @@ import org.apache.poi.util.LittleEndianOutput;
  * @author  andy
  * @author Jason Height (jheight at chariot dot net dot au)
  */
-public abstract class AreaPtgBase extends OperandPtg implements AreaI {
+public abstract class AreaPtgBase<Z extends AreaPtgBase<Z>> extends OperandPtg<Z> implements AreaI {
 	/**
 	 * TODO - (May-2008) fix subclasses of AreaPtg 'AreaN~' which are used in shared formulas.
 	 * see similar comment in ReferencePtg
@@ -96,17 +96,6 @@ public abstract class AreaPtgBase extends OperandPtg implements AreaI {
 		}
 	}
 
-	private static void $checkColumnBounds(int colIx) {
-		if((colIx & 0x0FF) != colIx) {
-			throw new IllegalArgumentException("colIx (" + colIx + ") is out of range");
-		}
-	}
-	private static void $checkRowBounds(int rowIx) {
-		if((rowIx & 0x0FFFF) != rowIx) {
-			throw new IllegalArgumentException("rowIx (" + rowIx + ") is out of range");
-		}
-	}
-
 	protected final void readCoordinates(LittleEndianInput in)  {
 		field_1_first_row = in.readUShort();
 		field_2_last_row = in.readUShort();

src/java/org/apache/poi/hssf/record/formula/AttrPtg.java

@@ -260,14 +260,4 @@ public final class AttrPtg extends ControlPtg {
       }
       return "UNKNOWN ATTRIBUTE";
      }
-
-    public Object clone() {
-        int[] jt;
-        if (_jumpTable == null) {
-            jt = null;
-        } else {
-            jt = _jumpTable.clone();
-        }
-        return new AttrPtg(_options, _data, jt, _chooseFuncOffset);
-    }
 }

src/java/org/apache/poi/hssf/record/formula/OperandPtg.java

@@ -21,12 +21,21 @@ package org.apache.poi.hssf.record.formula;
 /**
  * @author Josh Micich
  */
-public abstract class OperandPtg extends Ptg {
+public abstract class OperandPtg<Y extends OperandPtg<Y>> extends Ptg implements Cloneable {
 
 	/**
-	 * All Operand <tt>Ptg</tt>s are classifed ('relative', 'value', 'array')  
+	 * All Operand {@link Ptg}s are classified ('relative', 'value', 'array')
 	 */
 	public final boolean isBaseToken() {
 		return false;
 	}
+	public final Y copy() {
+		try {
+			@SuppressWarnings("unchecked")
+			Y result = (Y) clone();
+			return result;
+		} catch (CloneNotSupportedException e) {
+			throw new RuntimeException(e);
+		}
+	}
 }

src/java/org/apache/poi/hssf/record/formula/Ptg.java

@@ -40,7 +40,7 @@ import org.apache.poi.util.LittleEndianOutput;
  * @author avik
  * @author Jason Height (jheight at chariot dot net dot au)
  */
-public abstract class Ptg implements Cloneable {
+public abstract class Ptg {
 	public static final Ptg[] EMPTY_PTG_ARRAY = { };
 
 
@@ -158,28 +158,7 @@ public abstract class Ptg implements Cloneable {
 		}
 		throw new RuntimeException("Unexpected base token id (" + id + ")");
 	}
-	/**
-	 * @return a distinct copy of this <tt>Ptg</tt> if the class is mutable, or the same instance
-	 * if the class is immutable.
-	 */
-	public final Ptg copy() {
-		// TODO - all base tokens are logically immutable, but AttrPtg needs some clean-up
-		if (this instanceof ValueOperatorPtg) {
-			return this;
-		}
-		if (this instanceof ScalarConstantPtg) {
-			return this;
-		}
-		return (Ptg) clone();
-	}
 
-	protected Object clone() {
-		try {
-			return super.clone();
-		} catch (CloneNotSupportedException e) {
-			throw new RuntimeException(e);
-		}
-	}
 	private static Ptg[] toPtgArray(List<Ptg> l) {
 		if (l.isEmpty()) {
 			return EMPTY_PTG_ARRAY;

src/java/org/apache/poi/hssf/record/formula/Ref3DPtg.java

@@ -31,7 +31,7 @@ import org.apache.poi.util.LittleEndianOutput;
  * @author Libin Roman (Vista Portal LDT. Developer)
  * @author Jason Height (jheight at chariot dot net dot au)
  */
-public final class Ref3DPtg extends RefPtgBase implements WorkbookDependentFormula, ExternSheetReferenceToken {
+public final class Ref3DPtg extends RefPtgBase<Ref3DPtg> implements WorkbookDependentFormula, ExternSheetReferenceToken {
     public final static byte sid  = 0x3a;
 
     private final static int  SIZE = 7; // 6 + 1 for Ptg

src/java/org/apache/poi/hssf/record/formula/RefPtgBase.java

@@ -29,7 +29,7 @@ import org.apache.poi.util.LittleEndianOutput;
  * @author Andrew C. Oliver (acoliver@apache.org)
  * @author Jason Height (jheight at chariot dot net dot au)
  */
-public abstract class RefPtgBase extends OperandPtg {
+public abstract class RefPtgBase<Z extends RefPtgBase<Z>> extends OperandPtg<Z> {
 
 	/** The row index - zero based unsigned 16 bit value */
 	private int field_1_row;

src/java/org/apache/poi/hssf/record/formula/UnknownPtg.java

@@ -19,8 +19,7 @@ package org.apache.poi.hssf.record.formula;
 import org.apache.poi.util.LittleEndianOutput;
 
 /**
- *
+ * @author andy
- * @author  andy
  * @author Jason Height (jheight at chariot dot net dot au)
  */
 public class UnknownPtg extends Ptg {
@@ -38,18 +37,14 @@ public class UnknownPtg extends Ptg {
         out.writeByte(_sid);
     }
 
-    public int getSize()
+    public int getSize() {
-    {
         return size;
     }
 
-    public String toFormulaString()
+    public String toFormulaString() {
-    {
         return "UNKNOWN";
     }
-    public byte getDefaultOperandClass() {return Ptg.CLASS_VALUE;}
+    public byte getDefaultOperandClass() {
-
+        return Ptg.CLASS_VALUE;
-    public Object clone() {
-      return this;
     }
 }

src/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java

@@ -676,16 +676,16 @@ public class HSSFWorkbook extends POIDocument implements org.apache.poi.ss.userm
             Ptg[] ptgs = origNameRecord.getNameDefinition();
             for (int i=0; i< ptgs.length; i++) {
                 Ptg ptg = ptgs[i];
-                ptg = ptg.copy();
 
                 if (ptg instanceof Area3DPtg) {
-                    Area3DPtg a3p = (Area3DPtg) ptg;
+                    Area3DPtg a3p = ((Area3DPtg) ptg).copy();
                     a3p.setExternSheetIndex(newExtSheetIx);
+                    ptgs[i] = a3p;
                 } else if (ptg instanceof Ref3DPtg) {
-                    Ref3DPtg r3p = (Ref3DPtg) ptg;
+                    Ref3DPtg r3p = ((Ref3DPtg) ptg).copy();
                     r3p.setExternSheetIndex(newExtSheetIx);
+                    ptgs[i] = r3p;
                 }
-                ptgs[i] = ptg;
             }
             NameRecord newNameRecord = workbook.createBuiltInName(NameRecord.BUILTIN_FILTER_DB, newSheetIndex+1);
             newNameRecord.setNameDefinition(ptgs);

src/testcases/org/apache/poi/hssf/record/formula/TestFuncPtg.java

@@ -28,11 +28,8 @@ import org.apache.poi.hssf.record.TestcaseRecordInputStream;
 public final class TestFuncPtg extends TestCase {
 
     public void testRead() {
-    	// This ptg represents a LEN function extracted from excel
+    	// This function index represents the LEN() function
-        byte[] fakeData = {
+        byte[] fakeData = { 0x20, 0x00,};
-            0x20,  //function index
-            0,
-        };
 
         FuncPtg ptg = FuncPtg.create(TestcaseRecordInputStream.createLittleEndian(fakeData) );
         assertEquals( "Len formula index is not 32(20H)", 0x20, ptg.getFunctionIndex() );
@@ -41,14 +38,9 @@ public final class TestFuncPtg extends TestCase {
         assertEquals( "Ptg Size", 3, ptg.getSize() );
     }
 
-    public void testClone() {
+    public void testNumberOfOperands() {
         FuncPtg funcPtg = FuncPtg.create(27); // ROUND() - takes 2 args
-
+        assertEquals(2, funcPtg.getNumberOfOperands());
-        FuncPtg clone = (FuncPtg) funcPtg.clone();
+        assertEquals("ROUND", funcPtg.getName());
-        if (clone.getNumberOfOperands() == 0) {
-            fail("clone() did copy field numberOfOperands");
-        }
-        assertEquals(2, clone.getNumberOfOperands());
-        assertEquals("ROUND", clone.getName());
     }
 }