From 00e64461a912857c8ed3d1020a1774fe517e6b2d Mon Sep 17 00:00:00 2001 From: Josh Micich Date: Sat, 14 Nov 2009 02:41:24 +0000 Subject: [PATCH] Made AbstractFunctionPtg immutable, other minor improvements git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@836101 13f79535-47bb-0310-9956-ffa450edef68 --- .../record/formula/AbstractFunctionPtg.java | 54 ++++++++++------- .../poi/hssf/record/formula/FuncPtg.java | 51 +++++----------- .../poi/hssf/record/formula/FuncVarPtg.java | 58 +++++++------------ .../apache/poi/hssf/record/formula/Ptg.java | 4 +- .../apache/poi/ss/formula/FormulaParser.java | 8 +-- .../ss/formula/OperandClassTransformer.java | 50 ++++++++-------- .../poi/ss/formula/WorkbookEvaluator.java | 3 +- .../poi/hssf/model/TestFormulaParser.java | 2 +- .../poi/hssf/record/formula/TestFuncPtg.java | 8 +-- .../record/formula/eval/TestRangeEval.java | 2 +- 10 files changed, 106 insertions(+), 134 deletions(-) diff --git a/src/java/org/apache/poi/hssf/record/formula/AbstractFunctionPtg.java b/src/java/org/apache/poi/hssf/record/formula/AbstractFunctionPtg.java index 8e1f9ffe9..c6a0dedd2 100644 --- a/src/java/org/apache/poi/hssf/record/formula/AbstractFunctionPtg.java +++ b/src/java/org/apache/poi/hssf/record/formula/AbstractFunctionPtg.java @@ -37,45 +37,55 @@ public abstract class AbstractFunctionPtg extends OperationPtg { /** All external functions have function index 255 */ private static final short FUNCTION_INDEX_EXTERNAL = 255; - protected byte returnClass; - protected byte[] paramClass; + private final byte returnClass; + private final byte[] paramClass; - protected byte field_1_num_args; - protected short field_2_fnc_index; + private final byte _numberOfArgs; + private final short _functionIndex; - public final boolean isBaseToken() { - return false; + protected AbstractFunctionPtg(int functionIndex, int pReturnClass, byte[] paramTypes, int nParams) { + _numberOfArgs = (byte) nParams; + _functionIndex = (short) functionIndex; + returnClass = (byte) pReturnClass; + paramClass = paramTypes; } - - public String toString() { - StringBuffer sb = new StringBuffer(64); + public final boolean isBaseToken() { + return false; + } + + public final String toString() { + StringBuilder sb = new StringBuilder(64); sb.append(getClass().getName()).append(" ["); - sb.append(field_2_fnc_index).append(" ").append(field_1_num_args); + sb.append(lookupName(_functionIndex)); + sb.append(" nArgs=").append(_numberOfArgs); sb.append("]"); return sb.toString(); } - public short getFunctionIndex() { - return field_2_fnc_index; + public final short getFunctionIndex() { + return _functionIndex; + } + public final int getNumberOfOperands() { + return _numberOfArgs; } - public String getName() { - return lookupName(field_2_fnc_index); + public final String getName() { + return lookupName(_functionIndex); } /** * external functions get some special processing * @return true if this is an external function */ - public boolean isExternalFunction() { - return field_2_fnc_index == FUNCTION_INDEX_EXTERNAL; + public final boolean isExternalFunction() { + return _functionIndex == FUNCTION_INDEX_EXTERNAL; } - public String toFormulaString() { + public final String toFormulaString() { return getName(); } public String toFormulaString(String[] operands) { - StringBuffer buf = new StringBuffer(); + StringBuilder buf = new StringBuilder(); if(isExternalFunction()) { buf.append(operands[0]); // first operand is actually the function name @@ -87,7 +97,7 @@ public abstract class AbstractFunctionPtg extends OperationPtg { return buf.toString(); } - private static void appendArgs(StringBuffer buf, int firstArgIx, String[] operands) { + private static void appendArgs(StringBuilder buf, int firstArgIx, String[] operands) { buf.append('('); for (int i=firstArgIx;ifirstArgIx) { @@ -113,7 +123,7 @@ public abstract class AbstractFunctionPtg extends OperationPtg { return ix >= 0; } - protected String lookupName(short index) { + protected final String lookupName(short index) { if(index == FunctionMetadataRegistry.FUNCTION_INDEX_EXTERNAL) { return "#external#"; } @@ -142,10 +152,10 @@ public abstract class AbstractFunctionPtg extends OperationPtg { return returnClass; } - public byte getParameterClass(int index) { + public final byte getParameterClass(int index) { if (index >= paramClass.length) { // For var-arg (and other?) functions, the metadata does not list all the parameter - // operand classes. In these cases, all extra parameters are assumed to have the + // operand classes. In these cases, all extra parameters are assumed to have the // same operand class as the last one specified. return paramClass[paramClass.length - 1]; } diff --git a/src/java/org/apache/poi/hssf/record/formula/FuncPtg.java b/src/java/org/apache/poi/hssf/record/formula/FuncPtg.java index 133072203..4a2ca1bfc 100644 --- a/src/java/org/apache/poi/hssf/record/formula/FuncPtg.java +++ b/src/java/org/apache/poi/hssf/record/formula/FuncPtg.java @@ -16,6 +16,7 @@ ==================================================================== */ package org.apache.poi.hssf.record.formula; + import org.apache.poi.hssf.record.formula.function.FunctionMetadata; import org.apache.poi.hssf.record.formula.function.FunctionMetadataRegistry; import org.apache.poi.util.LittleEndianInput; @@ -30,50 +31,30 @@ public final class FuncPtg extends AbstractFunctionPtg { public final static byte sid = 0x21; public final static int SIZE = 3; - private int numParams=0; - /**Creates new function pointer from a byte array - * usually called while reading an excel file. - */ - public FuncPtg(LittleEndianInput in) { - //field_1_num_args = data[ offset + 0 ]; - field_2_fnc_index = in.readShort(); - - FunctionMetadata fm = FunctionMetadataRegistry.getFunctionByIndex(field_2_fnc_index); - if(fm == null) { - throw new RuntimeException("Invalid built-in function index (" + field_2_fnc_index + ")"); - } - numParams = fm.getMinParams(); - returnClass = fm.getReturnClassCode(); - paramClass = fm.getParameterClassCodes(); + public static FuncPtg create(LittleEndianInput in) { + return create(in.readUShort()); } - public FuncPtg(int functionIndex) { - field_2_fnc_index = (short) functionIndex; + + private FuncPtg(int funcIndex, FunctionMetadata fm) { + super(funcIndex, fm.getReturnClassCode(), fm.getParameterClassCodes(), fm.getMinParams()); // minParams same as max since these are not var-arg funcs + } + + public static FuncPtg create(int functionIndex) { FunctionMetadata fm = FunctionMetadataRegistry.getFunctionByIndex(functionIndex); - numParams = fm.getMinParams(); // same as max since these are not var-arg funcs - returnClass = fm.getReturnClassCode(); - paramClass = fm.getParameterClassCodes(); + if(fm == null) { + throw new RuntimeException("Invalid built-in function index (" + functionIndex + ")"); + } + return new FuncPtg(functionIndex, fm); } + public void write(LittleEndianOutput out) { out.writeByte(sid + getPtgClass()); - out.writeShort(field_2_fnc_index); - } - - public int getNumberOfOperands() { - return numParams; + out.writeShort(getFunctionIndex()); } public int getSize() { return SIZE; } - - public String toString() { - StringBuffer sb = new StringBuffer(64); - sb.append(getClass().getName()).append(" ["); - sb.append(lookupName(field_2_fnc_index)); - sb.append(" nArgs=").append(numParams); - sb.append("]"); - return sb.toString(); - } -} \ No newline at end of file +} diff --git a/src/java/org/apache/poi/hssf/record/formula/FuncVarPtg.java b/src/java/org/apache/poi/hssf/record/formula/FuncVarPtg.java index 1deb630d5..607a64f17 100644 --- a/src/java/org/apache/poi/hssf/record/formula/FuncVarPtg.java +++ b/src/java/org/apache/poi/hssf/record/formula/FuncVarPtg.java @@ -22,7 +22,6 @@ import org.apache.poi.util.LittleEndianInput; import org.apache.poi.util.LittleEndianOutput; /** - * * @author Jason Height (jheight at chariot dot net dot au) */ public final class FuncVarPtg extends AbstractFunctionPtg{ @@ -30,60 +29,45 @@ public final class FuncVarPtg extends AbstractFunctionPtg{ public final static byte sid = 0x22; private final static int SIZE = 4; + /** + * Single instance of this token for 'sum() taking a single argument' + */ + public static final OperationPtg SUM = FuncVarPtg.create("SUM", 1); + + private FuncVarPtg(int functionIndex, int returnClass, byte[] paramClasses, int numArgs) { + super(functionIndex, returnClass, paramClasses, numArgs); + } + /**Creates new function pointer from a byte array * usually called while reading an excel file. */ - public FuncVarPtg(LittleEndianInput in) { - field_1_num_args = in.readByte(); - field_2_fnc_index = in.readShort(); - FunctionMetadata fm = FunctionMetadataRegistry.getFunctionByIndex(field_2_fnc_index); - if(fm == null) { - // Happens only as a result of a call to FormulaParser.parse(), with a non-built-in function name - returnClass = Ptg.CLASS_VALUE; - paramClass = new byte[] {Ptg.CLASS_VALUE}; - } else { - returnClass = fm.getReturnClassCode(); - paramClass = fm.getParameterClassCodes(); - } + public static FuncVarPtg create(LittleEndianInput in) { + return create(in.readByte(), in.readShort()); } /** * Create a function ptg from a string tokenised by the parser */ - public FuncVarPtg(String pName, byte pNumOperands) { - field_1_num_args = pNumOperands; - field_2_fnc_index = lookupIndex(pName); - FunctionMetadata fm = FunctionMetadataRegistry.getFunctionByIndex(field_2_fnc_index); + public static FuncVarPtg create(String pName, int numArgs) { + return create(numArgs, lookupIndex(pName)); + } + + private static FuncVarPtg create(int numArgs, int functionIndex) { + FunctionMetadata fm = FunctionMetadataRegistry.getFunctionByIndex(functionIndex); if(fm == null) { // Happens only as a result of a call to FormulaParser.parse(), with a non-built-in function name - returnClass = Ptg.CLASS_VALUE; - paramClass = new byte[] {Ptg.CLASS_VALUE}; - } else { - returnClass = fm.getReturnClassCode(); - paramClass = fm.getParameterClassCodes(); + return new FuncVarPtg(functionIndex, Ptg.CLASS_VALUE, new byte[] {Ptg.CLASS_VALUE}, numArgs); } + return new FuncVarPtg(functionIndex, fm.getReturnClassCode(), fm.getParameterClassCodes(), numArgs); } public void write(LittleEndianOutput out) { out.writeByte(sid + getPtgClass()); - out.writeByte(field_1_num_args); - out.writeShort(field_2_fnc_index); - } - - public int getNumberOfOperands() { - return field_1_num_args; + out.writeByte(getNumberOfOperands()); + out.writeShort(getFunctionIndex()); } public int getSize() { return SIZE; } - - public String toString() { - StringBuffer sb = new StringBuffer(64); - sb.append(getClass().getName()).append(" ["); - sb.append(lookupName(field_2_fnc_index)); - sb.append(" nArgs=").append(field_1_num_args); - sb.append("]"); - return sb.toString(); - } } diff --git a/src/java/org/apache/poi/hssf/record/formula/Ptg.java b/src/java/org/apache/poi/hssf/record/formula/Ptg.java index 140e366bb..7e2ad66a4 100644 --- a/src/java/org/apache/poi/hssf/record/formula/Ptg.java +++ b/src/java/org/apache/poi/hssf/record/formula/Ptg.java @@ -102,8 +102,8 @@ public abstract class Ptg implements Cloneable { switch (baseId) { case ArrayPtg.sid: return new ArrayPtg(in); // 0x20, 0x40, 0x60 - case FuncPtg.sid: return new FuncPtg(in); // 0x21, 0x41, 0x61 - case FuncVarPtg.sid: return new FuncVarPtg(in); // 0x22, 0x42, 0x62 + case FuncPtg.sid: return FuncPtg.create(in); // 0x21, 0x41, 0x61 + case FuncVarPtg.sid: return FuncVarPtg.create(in);//0x22, 0x42, 0x62 case NamePtg.sid: return new NamePtg(in); // 0x23, 0x43, 0x63 case RefPtg.sid: return new RefPtg(in); // 0x24, 0x44, 0x64 case AreaPtg.sid: return new AreaPtg(in); // 0x25, 0x45, 0x65 diff --git a/src/java/org/apache/poi/ss/formula/FormulaParser.java b/src/java/org/apache/poi/ss/formula/FormulaParser.java index ec2252d5e..0a7b56812 100644 --- a/src/java/org/apache/poi/ss/formula/FormulaParser.java +++ b/src/java/org/apache/poi/ss/formula/FormulaParser.java @@ -970,7 +970,7 @@ public final class FormulaParser { ParseNode[] allArgs = new ParseNode[numArgs+1]; allArgs[0] = new ParseNode(namePtg); System.arraycopy(args, 0, allArgs, 1, numArgs); - return new ParseNode(new FuncVarPtg(name, (byte)(numArgs+1)), allArgs); + return new ParseNode(FuncVarPtg.create(name, numArgs+1), allArgs); } if (namePtg != null) { @@ -988,9 +988,9 @@ public final class FormulaParser { AbstractFunctionPtg retval; if(isVarArgs) { - retval = new FuncVarPtg(name, (byte)numArgs); + retval = FuncVarPtg.create(name, numArgs); } else { - retval = new FuncPtg(funcIx); + retval = FuncPtg.create(funcIx); } return new ParseNode(retval, args); } @@ -1013,7 +1013,7 @@ public final class FormulaParser { maxArgs = _book.getSpreadsheetVersion().getMaxFunctionArgs(); } else { //_book can be omitted by test cases - maxArgs = fm.getMaxParams(); // just use BIFF8 + maxArgs = fm.getMaxParams(); // just use BIFF8 } } else { maxArgs = fm.getMaxParams(); diff --git a/src/java/org/apache/poi/ss/formula/OperandClassTransformer.java b/src/java/org/apache/poi/ss/formula/OperandClassTransformer.java index 620370aa0..9af641128 100644 --- a/src/java/org/apache/poi/ss/formula/OperandClassTransformer.java +++ b/src/java/org/apache/poi/ss/formula/OperandClassTransformer.java @@ -29,29 +29,29 @@ import org.apache.poi.hssf.record.formula.UnionPtg; import org.apache.poi.hssf.record.formula.ValueOperatorPtg; /** - * This class performs 'operand class' transformation. Non-base tokens are classified into three + * This class performs 'operand class' transformation. Non-base tokens are classified into three * operand classes: * *

- * + * * The final operand class chosen for each token depends on the formula type and the token's place * in the formula. If POI gets the operand class wrong, Excel may interpret the formula * incorrectly. This condition is typically manifested as a formula cell that displays as '#VALUE!', * but resolves correctly when the user presses F2, enter.

- * + * * The logic implemented here was partially inspired by the description in * "OpenOffice.org's Documentation of the Microsoft Excel File Format". The model presented there * seems to be inconsistent with observed Excel behaviour (These differences have not been fully * investigated). The implementation in this class has been heavily modified in order to satisfy * concrete examples of how Excel performs the same logic (see TestRVA).

- * - * Hopefully, as additional important test cases are identified and added to the test suite, + * + * Hopefully, as additional important test cases are identified and added to the test suite, * patterns might become more obvious in this code and allow for simplification. - * + * * @author Josh Micich */ final class OperandClassTransformer { @@ -77,15 +77,15 @@ final class OperandClassTransformer { rootNodeOperandClass = Ptg.CLASS_REF; break; default: - throw new RuntimeException("Incomplete code - formula type (" + throw new RuntimeException("Incomplete code - formula type (" + _formulaType + ") not supported yet"); - + } transformNode(rootNode, rootNodeOperandClass, false); } /** - * @param callerForceArrayFlag true if one of the current node's parents is a + * @param callerForceArrayFlag true if one of the current node's parents is a * function Ptg which has been changed from default 'V' to 'A' type (due to requirements on * the function return value). */ @@ -94,7 +94,7 @@ final class OperandClassTransformer { Ptg token = node.getToken(); ParseNode[] children = node.getChildren(); boolean isSimpleValueFunc = isSimpleValueFunction(token); - + if (isSimpleValueFunc) { boolean localForceArray = desiredOperandClass == Ptg.CLASS_ARRAY; for (int i = 0; i < children.length; i++) { @@ -103,12 +103,12 @@ final class OperandClassTransformer { setSimpleValueFuncClass((AbstractFunctionPtg) token, desiredOperandClass, callerForceArrayFlag); return; } - + if (isSingleArgSum(token)) { // Need to process the argument of SUM with transformFunctionNode below // so make a dummy FuncVarPtg for that call. - token = new FuncVarPtg("SUM", (byte)1); - // Note - the tAttrSum token (node.getToken()) is a base + token = FuncVarPtg.SUM; + // Note - the tAttrSum token (node.getToken()) is a base // token so does not need to have its operand class set } if (token instanceof ValueOperatorPtg || token instanceof ControlPtg @@ -117,9 +117,9 @@ final class OperandClassTransformer { || token instanceof UnionPtg) { // Value Operator Ptgs and Control are base tokens, so token will be unchanged // but any child nodes are processed according to desiredOperandClass and callerForceArrayFlag - + // As per OOO documentation Sec 3.2.4 "Token Class Transformation", "Step 1" - // All direct operands of value operators that are initially 'R' type will + // All direct operands of value operators that are initially 'R' type will // be converted to 'V' type. byte localDesiredOperandClass = desiredOperandClass == Ptg.CLASS_REF ? Ptg.CLASS_VALUE : desiredOperandClass; for (int i = 0; i < children.length; i++) { @@ -149,7 +149,7 @@ final class OperandClassTransformer { private static boolean isSingleArgSum(Ptg token) { if (token instanceof AttrPtg) { AttrPtg attrPtg = (AttrPtg) token; - return attrPtg.isSum(); + return attrPtg.isSum(); } return false; } @@ -180,12 +180,12 @@ final class OperandClassTransformer { } // else fall through case Ptg.CLASS_ARRAY: - return Ptg.CLASS_ARRAY; + return Ptg.CLASS_ARRAY; case Ptg.CLASS_REF: if (!callerForceArrayFlag) { return currentOperandClass; } - return Ptg.CLASS_REF; + return Ptg.CLASS_REF; } throw new IllegalStateException("Unexpected operand class (" + desiredOperandClass + ")"); } @@ -221,15 +221,15 @@ final class OperandClassTransformer { } else { if (defaultReturnOperandClass == desiredOperandClass) { localForceArrayFlag = false; - // an alternative would have been to for non-base Ptgs to set their operand class + // an alternative would have been to for non-base Ptgs to set their operand class // from their default, but this would require the call in many subclasses because // the default OC is not known until the end of the constructor - afp.setClass(defaultReturnOperandClass); + afp.setClass(defaultReturnOperandClass); } else { switch (desiredOperandClass) { case Ptg.CLASS_VALUE: // always OK to set functions to return 'value' - afp.setClass(Ptg.CLASS_VALUE); + afp.setClass(Ptg.CLASS_VALUE); localForceArrayFlag = false; break; case Ptg.CLASS_ARRAY: @@ -282,7 +282,7 @@ final class OperandClassTransformer { if (callerForceArrayFlag || desiredOperandClass == Ptg.CLASS_ARRAY) { afp.setClass(Ptg.CLASS_ARRAY); } else { - afp.setClass(Ptg.CLASS_VALUE); + afp.setClass(Ptg.CLASS_VALUE); } } } diff --git a/src/java/org/apache/poi/ss/formula/WorkbookEvaluator.java b/src/java/org/apache/poi/ss/formula/WorkbookEvaluator.java index f9b7151df..18d8c11d4 100644 --- a/src/java/org/apache/poi/ss/formula/WorkbookEvaluator.java +++ b/src/java/org/apache/poi/ss/formula/WorkbookEvaluator.java @@ -344,8 +344,7 @@ public final class WorkbookEvaluator { if (attrPtg.isSum()) { // Excel prefers to encode 'SUM()' as a tAttr token, but this evaluator // expects the equivalent function token - byte nArgs = 1; // tAttrSum always has 1 parameter - ptg = new FuncVarPtg("SUM", nArgs); + ptg = FuncVarPtg.SUM; } if (attrPtg.isOptimizedChoose()) { ValueEval arg0 = stack.pop(); diff --git a/src/testcases/org/apache/poi/hssf/model/TestFormulaParser.java b/src/testcases/org/apache/poi/hssf/model/TestFormulaParser.java index 30c6df530..6af5ba4ba 100644 --- a/src/testcases/org/apache/poi/hssf/model/TestFormulaParser.java +++ b/src/testcases/org/apache/poi/hssf/model/TestFormulaParser.java @@ -419,7 +419,7 @@ public final class TestFormulaParser extends TestCase { HSSFWorkbook book = new HSSFWorkbook(); Ptg[] ptgs = { - new FuncPtg(10), + FuncPtg.create(10), }; assertEquals("NA()", HSSFFormulaParser.toFormulaString(book, ptgs)); } diff --git a/src/testcases/org/apache/poi/hssf/record/formula/TestFuncPtg.java b/src/testcases/org/apache/poi/hssf/record/formula/TestFuncPtg.java index bbf8c4cfb..399e80c24 100644 --- a/src/testcases/org/apache/poi/hssf/record/formula/TestFuncPtg.java +++ b/src/testcases/org/apache/poi/hssf/record/formula/TestFuncPtg.java @@ -34,15 +34,15 @@ public final class TestFuncPtg extends TestCase { 0, }; - FuncPtg ptg = new FuncPtg(TestcaseRecordInputStream.createLittleEndian(fakeData) ); + FuncPtg ptg = FuncPtg.create(TestcaseRecordInputStream.createLittleEndian(fakeData) ); assertEquals( "Len formula index is not 32(20H)", 0x20, ptg.getFunctionIndex() ); assertEquals( "Number of operands in the len formula", 1, ptg.getNumberOfOperands() ); assertEquals( "Function Name", "LEN", ptg.getName() ); assertEquals( "Ptg Size", 3, ptg.getSize() ); } - + public void testClone() { - FuncPtg funcPtg = new FuncPtg(27); // ROUND() - takes 2 args + FuncPtg funcPtg = FuncPtg.create(27); // ROUND() - takes 2 args FuncPtg clone = (FuncPtg) funcPtg.clone(); if (clone.getNumberOfOperands() == 0) { @@ -52,5 +52,3 @@ public final class TestFuncPtg extends TestCase { assertEquals("ROUND", clone.getName()); } } - - diff --git a/src/testcases/org/apache/poi/hssf/record/formula/eval/TestRangeEval.java b/src/testcases/org/apache/poi/hssf/record/formula/eval/TestRangeEval.java index dcf53132e..07e40898c 100644 --- a/src/testcases/org/apache/poi/hssf/record/formula/eval/TestRangeEval.java +++ b/src/testcases/org/apache/poi/hssf/record/formula/eval/TestRangeEval.java @@ -186,7 +186,7 @@ public final class TestRangeEval extends TestCase { new RefPtg("C1"), new IntPtg(0), new RefPtg("B1"), - new FuncVarPtg("OFFSET", (byte)3), + FuncVarPtg.create("OFFSET", (byte)3), RangePtg.instance, AttrPtg.SUM, };