Improvements to formula evaluation treatment of -0.0. (Refinements to fix for bug 47198

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

src/java/org/apache/poi/hssf/record/formula/eval/PercentEval.java

@@ -34,14 +34,17 @@ public final class PercentEval implements OperationEval {
 		if (args.length != 1) {
 			return ErrorEval.VALUE_INVALID;
 		}
-		double d0;
+		double d;
 		try {
 			ValueEval ve = OperandResolver.getSingleValue(args[0], srcRow, srcCol);
-			d0 = OperandResolver.coerceValueToDouble(ve);
+			d = OperandResolver.coerceValueToDouble(ve);
 		} catch (EvaluationException e) {
 			return e.getErrorEval();
 		}
-		return new NumberEval(d0 / 100);
+		if (d == 0.0) { // this '==' matches +0.0 and -0.0
+			return NumberEval.ZERO;
+		}
+		return new NumberEval(d / 100);
 	}
 
 	public int getNumberOfOperands() {

src/java/org/apache/poi/hssf/record/formula/eval/RelationalOperationEval.java

@@ -108,10 +108,7 @@ public abstract class RelationalOperationEval implements OperationEval {
 			if (vb instanceof NumberEval) {
 				NumberEval nA = (NumberEval) va;
 				NumberEval nB = (NumberEval) vb;
-				if (nA.getNumberValue() == nB.getNumberValue()) {
+				// Excel considers -0.0 < 0.0 which is the same as Double.compare()
-					// Excel considers -0.0 == 0.0 which is different to Double.compare()
-					return 0;
-				}
 				return Double.compare(nA.getNumberValue(), nB.getNumberValue());
 			}
 		}

src/java/org/apache/poi/hssf/record/formula/eval/TwoOperandNumericOperation.java

@@ -33,6 +33,12 @@ abstract class TwoOperandNumericOperation implements OperationEval {
 			double d0 = singleOperandEvaluate(args[0], srcCellRow, srcCellCol);
 			double d1 = singleOperandEvaluate(args[1], srcCellRow, srcCellCol);
 			result = evaluate(d0, d1);
+			if (result == 0.0) { // this '==' matches +0.0 and -0.0
+				// Excel converts -0.0 to +0.0 for '*', '/', '%', '+' and '^'
+				if (!(this instanceof SubtractEval)) {
+					return NumberEval.ZERO;
+				}
+			}
 			if (Double.isNaN(result) || Double.isInfinite(result)) {
 				return ErrorEval.NUM_ERROR;
 			}

src/java/org/apache/poi/hssf/record/formula/eval/UnaryMinusEval.java

@@ -41,6 +41,9 @@ public final class UnaryMinusEval implements OperationEval {
 		} catch (EvaluationException e) {
 			return e.getErrorEval();
 		}
+		if (d == 0.0) { // this '==' matches +0.0 and -0.0
+			return NumberEval.ZERO;
+		}
 		return new NumberEval(-d);
 	}
 

src/java/org/apache/poi/ss/util/NumberToTextConverter.java

@@ -154,7 +154,7 @@ public final class NumberToTextConverter {
 		if (biasedExponent == 0) {
 			// value is 'denormalised' which means it is less than 2^-1022
 			// excel displays all these numbers as zero, even though calculations work OK
-			return "0";
+			return isNegative ? "-0" : "0";
 		}
 
 		int exponent = biasedExponent - EXPONENT_BIAS;

src/testcases/org/apache/poi/hssf/record/formula/eval/AllFormulaEvalTests.java

@@ -36,6 +36,7 @@ public class AllFormulaEvalTests {
 		result.addTestSuite(TestExternalFunction.class);
 		result.addTestSuite(TestFormulaBugs.class);
 		result.addTestSuite(TestFormulasFromSpreadsheet.class);
+		result.addTestSuite(TestMinusZeroResult.class);
 		result.addTestSuite(TestMissingArgEval.class);
 		result.addTestSuite(TestPercentEval.class);
 		result.addTestSuite(TestRangeEval.class);

src/testcases/org/apache/poi/hssf/record/formula/eval/TestEqualEval.java

@@ -93,12 +93,21 @@ public final class TestEqualEval extends TestCase {
 	}
 
 	/**
-	 * Excel considers -0.0 to be equal to 0.0
+	 * Bug 47198 involved a formula "-A1=0" where cell A1 was 0.0.
+	 * Excel evaluates "-A1=0" to TRUE, not because it thinks -0.0==0.0 
+	 * but because "-A1" evaluated to +0.0
+	 * <p/>
+	 * Note - the original diagnosis of bug 47198 was that 
+	 * "Excel considers -0.0 to be equal to 0.0" which is NQR
+	 * See {@link TestMinusZeroResult} for more specific tests regarding -0.0.
 	 */
 	public void testZeroEquality_bug47198() {
 		NumberEval zero = new NumberEval(0.0);
 		NumberEval mZero = (NumberEval) UnaryMinusEval.instance.evaluate(new Eval[] { zero, }, 0,
 				(short) 0);
+		if (Double.doubleToLongBits(mZero.getNumberValue()) == 0x8000000000000000L) {
+			throw new AssertionFailedError("Identified bug 47198: unary minus should convert -0.0 to 0.0");
+		}
 		Eval[] args = { zero, mZero, };
 		BoolEval result = (BoolEval) EqualEval.instance.evaluate(args, 0, (short) 0);
 		if (!result.getBooleanValue()) {

src/testcases/org/apache/poi/hssf/record/formula/eval/TestMinusZeroResult.java

@@ -0,0 +1,148 @@
+/* ====================================================================
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+==================================================================== */
+
+package org.apache.poi.hssf.record.formula.eval;
+
+import junit.framework.ComparisonFailure;
+import junit.framework.TestCase;
+
+import org.apache.poi.util.HexDump;
+
+/**
+ * IEEE 754 defines a quantity '-0.0' which is distinct from '0.0'.
+ * Negative zero is not easy to observe in Excel, since it is usually converted to 0.0.
+ * (Note - the results of XLL add-in functions don't seem to be converted, so they are one
+ * reliable avenue to observe Excel's treatment of '-0.0' as an operand.)
+ * <p/>
+ * POI attempts to emulate Excel faithfully, so this class tests
+ * two aspects of '-0.0' in formula evaluation:
+ * <ol>
+ * <li>For most operation results '-0.0' is converted to '0.0'.</li>
+ * <li>Comparison operators have slightly different rules regarding '-0.0'.</li>
+ * </ol>
+ * @author Josh Micich
+ */
+public final class TestMinusZeroResult extends TestCase {
+	private static final double MINUS_ZERO = -0.0;
+
+
+	public void testSimpleOperators() {
+
+		// unary plus is a no-op
+		checkEval(MINUS_ZERO, UnaryPlusEval.instance, MINUS_ZERO);
+
+		// most simple operators convert -0.0 to +0.0
+		checkEval(0.0, UnaryMinusEval.instance, 0.0);
+		checkEval(0.0, PercentEval.instance, MINUS_ZERO);
+		checkEval(0.0, MultiplyEval.instance, MINUS_ZERO, 1.0);
+		checkEval(0.0, DivideEval.instance, MINUS_ZERO, 1.0);
+		checkEval(0.0, PowerEval.instance, MINUS_ZERO, 1.0);
+
+		// but SubtractEval does not convert -0.0, so '-' and '+' work like java
+		checkEval(MINUS_ZERO, SubtractEval.instance, MINUS_ZERO, 0.0); // this is the main point of bug 47198
+		checkEval(0.0, AddEval.instance, MINUS_ZERO, 0.0);
+	}
+
+	/**
+	 * These results are hard to see in Excel (since -0.0 is usually converted to +0.0 before it
+	 * gets to the comparison operator)
+	 */
+	public void testComparisonOperators() {
+		checkEval(false, EqualEval.instance, 0.0, MINUS_ZERO);
+		checkEval(true, GreaterThanEval.instance, 0.0, MINUS_ZERO);
+		checkEval(true, LessThanEval.instance, MINUS_ZERO, 0.0);
+	}
+
+	public void testTextRendering() {
+		confirmTextRendering("-0", MINUS_ZERO);
+		// sub-normal negative numbers also display as '-0'
+		confirmTextRendering("-0", Double.longBitsToDouble(0x8000100020003000L));
+	}
+
+	/**
+	 * Uses {@link ConcatEval} to force number-to-text conversion
+	 */
+	private static void confirmTextRendering(String expRendering, double d) {
+		Eval[] args = { StringEval.EMPTY_INSTANCE, new NumberEval(d), };
+		StringEval se = (StringEval) ConcatEval.instance.evaluate(args, -1, (short)-1);
+		String result = se.getStringValue();
+		assertEquals(expRendering, result);
+	}
+
+	private static void checkEval(double expectedResult, OperationEval instance, double... dArgs) {
+		NumberEval result = (NumberEval) evaluate(instance, dArgs);
+		assertDouble(expectedResult, result.getNumberValue());
+	}
+	private static void checkEval(boolean expectedResult, OperationEval instance, double... dArgs) {
+		BoolEval result = (BoolEval) evaluate(instance, dArgs);
+		assertEquals(expectedResult, result.getBooleanValue());
+	}
+	private static Eval evaluate(OperationEval instance, double... dArgs) {
+		Eval[] evalArgs;
+		evalArgs = new Eval[dArgs.length];
+		for (int i = 0; i < evalArgs.length; i++) {
+			evalArgs[i] = new NumberEval(dArgs[i]);
+		}
+		Eval r = instance.evaluate(evalArgs, -1, (short)-1);
+		return r;
+	}
+
+	/**
+	 * Not really a POI test - just shows similar behaviour of '-0.0' in Java.
+	 */
+	public void testJava() {
+
+		assertEquals(0x8000000000000000L, Double.doubleToLongBits(MINUS_ZERO));
+
+		// The simple operators consider all zeros to be the same
+		assertTrue(MINUS_ZERO == MINUS_ZERO);
+		assertTrue(MINUS_ZERO == +0.0);
+		assertFalse(MINUS_ZERO < +0.0);
+
+		// Double.compare() considers them different
+		assertTrue(Double.compare(MINUS_ZERO, +0.0) < 0);
+
+		// multiplying zero by any negative quantity yields minus zero
+		assertDouble(MINUS_ZERO, 0.0*-1);
+		assertDouble(MINUS_ZERO, 0.0*-1e300);
+		assertDouble(MINUS_ZERO, 0.0*-1e-300);
+
+		// minus zero can be produced as a result of underflow
+		assertDouble(MINUS_ZERO, -1e-300 / 1e100);
+
+		// multiplying or dividing minus zero by a positive quantity yields minus zero
+		assertDouble(MINUS_ZERO, MINUS_ZERO * 1.0);
+		assertDouble(MINUS_ZERO, MINUS_ZERO / 1.0);
+
+		// subtracting positive zero gives minus zero
+		assertDouble(MINUS_ZERO, MINUS_ZERO - 0.0);
+		// BUT adding positive zero gives positive zero
+		assertDouble(0.0, MINUS_ZERO + 0.0);  // <<----
+	}
+
+	/**
+	 * Just so there is no ambiguity.  The two double values have to be exactly equal
+	 */
+	private static void assertDouble(double a, double b) {
+		long bitsA = Double.doubleToLongBits(a);
+		long bitsB = Double.doubleToLongBits(b);
+		if (bitsA != bitsB) {
+			throw new ComparisonFailure("value different to expected",
+					new String(HexDump.longToHex(bitsA)), new String(HexDump.longToHex(bitsB)));
+		}
+	}
+}