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

@@ -19,7 +19,7 @@ package org.apache.poi.hssf.record.formula.eval;
 
 /**
  * Base class for all comparison operator evaluators
- * 
+ *
  * @author Amol S. Deshmukh < amolweb at ya hoo dot com >
  */
 public abstract class RelationalOperationEval implements OperationEval {
@@ -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 different to Double.compare()
-					return 0;
-				}
+				// Excel considers -0.0 < 0.0 which is the same as Double.compare()
 				return Double.compare(nA.getNumberValue(), nB.getNumberValue());
 			}
 		}

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

@@ -26,13 +26,19 @@ abstract class TwoOperandNumericOperation implements OperationEval {
 		ValueEval ve = OperandResolver.getSingleValue(arg, srcCellRow, srcCellCol);
 		return OperandResolver.coerceValueToDouble(ve);
 	}
-	
+
 	public final Eval evaluate(Eval[] args, int srcCellRow, short srcCellCol) {
 		double result;
 		try {
 			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

@@ -20,7 +20,7 @@ package org.apache.poi.hssf.record.formula.eval;
 
 /**
  * @author Amol S. Deshmukh < amolweb at ya hoo dot com >
- *  
+ *
  */
 public final class UnaryMinusEval implements OperationEval {
 
@@ -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

@@ -26,16 +26,16 @@ import java.math.BigInteger;
  * <ul>
  * <li>No more than 15 significant figures are output (java does 18).</li>
  * <li>The sign char for the exponent is included even if positive</li>
- * <li>Special values (<tt>NaN</tt> and <tt>Infinity</tt>) get rendered like the ordinary 
+ * <li>Special values (<tt>NaN</tt> and <tt>Infinity</tt>) get rendered like the ordinary
  * number that the bit pattern represents.</li>
  * <li>Denormalised values (between &plusmn;2<sup>-1074</sup> and &plusmn;2<sup>-1022</sup>
  *  are displayed as "0"</sup>
  * </ul>
  * IEEE 64-bit Double Rendering Comparison
- * 
+ *
  * <table border="1" cellpadding="2" cellspacing="0" summary="IEEE 64-bit Double Rendering Comparison">
  * <tr><th>Raw bits</th><th>Java</th><th>Excel</th></tr>
- * 
+ *
  * <tr><td>0x0000000000000000L</td><td>0.0</td><td>0</td></tr>
  * <tr><td>0x3FF0000000000000L</td><td>1.0</td><td>1</td></tr>
  * <tr><td>0x3FF00068DB8BAC71L</td><td>1.0001</td><td>1.0001</td></tr>
@@ -96,8 +96,8 @@ import java.math.BigInteger;
  * <tr><td>0x7FFFFFFFFFFFFFFFL</td><td>NaN</td><td>3.5953862697246E+308</td></tr>
  * <tr><td>0xFFF7FFFFFFFFFFFFL</td><td>NaN</td><td>2.6965397022935E+308</td></tr>
  * </table>
- * 
- * <b>Note</b>:  
+ *
+ * <b>Note</b>:
  * Excel has inconsistent rules for the following numeric operations:
  * <ul>
  * <li>Conversion to string (as handled here)</li>
@@ -105,10 +105,10 @@ import java.math.BigInteger;
  * <li>Conversion from text</li>
  * <li>General arithmetic</li>
  * </ul>
- * Excel's text to number conversion is not a true <i>inverse</i> of this operation.  The 
+ * Excel's text to number conversion is not a true <i>inverse</i> of this operation.  The
  * allowable ranges are different.  Some numbers that don't correctly convert to text actually
  * <b>do</b> get handled properly when used in arithmetic evaluations.
- * 
+ *
  * @author Josh Micich
  */
 public final class NumberToTextConverter {
@@ -119,49 +119,49 @@ public final class NumberToTextConverter {
 	private static final int  FRAC_BITS_WIDTH = EXPONENT_SHIFT;
 	private static final int  EXPONENT_BIAS  = 1023;
 	private static final long FRAC_ASSUMED_HIGH_BIT = ( 1L<<EXPONENT_SHIFT );
-	
+
 	private static final long EXCEL_NAN_BITS = 0xFFFF0420003C0000L;
 	private static final int MAX_TEXT_LEN = 20;
-	
+
 	private static final int DEFAULT_COUNT_SIGNIFICANT_DIGITS = 15;
 	private static final int MAX_EXTRA_ZEROS = MAX_TEXT_LEN - DEFAULT_COUNT_SIGNIFICANT_DIGITS;
 	private static final float LOG2_10 = 3.32F;
-	
+
 
 	private NumberToTextConverter() {
 		// no instances of this class
 	}
 
 	/**
-	 * Converts the supplied <tt>value</tt> to the text representation that Excel would give if 
+	 * Converts the supplied <tt>value</tt> to the text representation that Excel would give if
 	 * the value were to appear in an unformatted cell, or as a literal number in a formula.<br/>
 	 * Note - the results from this method differ slightly from those of <tt>Double.toString()</tt>
 	 * In some special cases Excel behaves quite differently.  This function attempts to reproduce
-	 * those results. 
+	 * those results.
 	 */
 	public static String toText(double value) {
 		return rawDoubleBitsToText(Double.doubleToLongBits(value));
 	}
 	/* package */ static String rawDoubleBitsToText(long pRawBits) {
-		
+
 		long rawBits = pRawBits;
 		boolean isNegative = rawBits < 0; // sign bit is in the same place for long and double
 		if (isNegative) {
 			rawBits &= 0x7FFFFFFFFFFFFFFFL;
 		}
-		
+
 		int biasedExponent = (int) ((rawBits & expMask) >> EXPONENT_SHIFT);
 		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; 
-		
+
+		int exponent = biasedExponent - EXPONENT_BIAS;
+
 		long fracBits = FRAC_ASSUMED_HIGH_BIT | rawBits & FRAC_MASK;
-		
-		
+
+
 		// Start by converting double value to BigDecimal
 		BigDecimal bd;
 		if (biasedExponent == 0x07FF) {
@@ -175,26 +175,26 @@ public final class NumberToTextConverter {
 			isNegative = false; // except that the sign bit is ignored
 		}
 		bd = convertToBigDecimal(exponent, fracBits);
-		
+
 		return formatBigInteger(isNegative, bd.unscaledValue(), bd.scale());
 	}
 
 	private static BigDecimal convertToBigDecimal(int exponent, long fracBits) {
 		byte[] joob = {
-				(byte) (fracBits >> 48), 	
-				(byte) (fracBits >> 40), 	
-				(byte) (fracBits >> 32), 	
-				(byte) (fracBits >> 24), 	
-				(byte) (fracBits >> 16), 	
-				(byte) (fracBits >>  8), 	
-				(byte) (fracBits >>  0), 	
+				(byte) (fracBits >> 48),
+				(byte) (fracBits >> 40),
+				(byte) (fracBits >> 32),
+				(byte) (fracBits >> 24),
+				(byte) (fracBits >> 16),
+				(byte) (fracBits >>  8),
+				(byte) (fracBits >>  0),
 		};
-		
+
 		BigInteger bigInt = new BigInteger(joob);
 		int lastSigBitIndex = exponent-FRAC_BITS_WIDTH;
 		if(lastSigBitIndex < 0) {
 			BigInteger shifto = new BigInteger("1").shiftLeft(-lastSigBitIndex);
-			int scale = 1 -(int) (lastSigBitIndex/LOG2_10); 
+			int scale = 1 -(int) (lastSigBitIndex/LOG2_10);
 			BigDecimal bd1 = new BigDecimal(bigInt);
 			BigDecimal bdShifto = new BigDecimal(shifto);
 			return bd1.divide(bdShifto, scale, BigDecimal.ROUND_HALF_UP);
@@ -208,10 +208,10 @@ public final class NumberToTextConverter {
 		if (scale < 0) {
 			throw new RuntimeException("negative scale");
 		}
-		
+
 		StringBuffer sb = new StringBuffer(unscaledValue.toString());
 		int numberOfLeadingZeros = -1;
-		
+
 		int unscaledLength = sb.length();
 		if (scale > 0 && scale >= unscaledLength) {
 			// less than one
@@ -226,7 +226,7 @@ public final class NumberToTextConverter {
 		}
 		return sb.toString();
 	}
-	
+
 	private static int getNumberOfSignificantFiguresDisplayed(int exponent) {
 		int nLostDigits; // number of significand digits lost due big exponents
 		if(exponent > 99) {
@@ -241,19 +241,19 @@ public final class NumberToTextConverter {
 		}
 		return DEFAULT_COUNT_SIGNIFICANT_DIGITS - nLostDigits;
 	}
-	
+
 	private static boolean needsScientificNotation(int nDigits) {
 		return nDigits > MAX_TEXT_LEN;
 	}
 
 	private static void formatGreaterThanOne(StringBuffer sb, int nIntegerDigits) {
-		
+
 		int maxSigFigs = getNumberOfSignificantFiguresDisplayed(nIntegerDigits);
 		int decimalPointIndex = nIntegerDigits;
 		boolean roundCausedCarry = performRound(sb, 0, maxSigFigs);
 
 		int endIx = Math.min(maxSigFigs, sb.length()-1);
-		
+
 		int nSigFigures;
 		if(roundCausedCarry) {
 			sb.insert(0, '1');
@@ -292,11 +292,11 @@ public final class NumberToTextConverter {
 		if (pAbsExponent < 1) {
 			throw new IllegalArgumentException("abs(exponent) must be positive");
 		}
-		
+
 		int numberOfLeadingZeros = pAbsExponent-1;
-		int absExponent = pAbsExponent; 
-		int maxSigFigs = getNumberOfSignificantFiguresDisplayed(-absExponent); 
-		
+		int absExponent = pAbsExponent;
+		int maxSigFigs = getNumberOfSignificantFiguresDisplayed(-absExponent);
+
 		boolean roundCausedCarry = performRound(sb, 0, maxSigFigs);
 		int nRemainingSigFigs;
 		if(roundCausedCarry) {
@@ -309,9 +309,9 @@ public final class NumberToTextConverter {
 			nRemainingSigFigs = countSignifantDigits(sb, 0 + maxSigFigs);
 			sb.setLength(nRemainingSigFigs);
 		}
-		
+
 		int normalLength = 2 + numberOfLeadingZeros + nRemainingSigFigs; // 2 == "0.".length()
-		
+
 		if (needsScientificNotation(normalLength)) {
 			if (sb.length()>1) {
 				sb.insert(1, '.');
@@ -319,7 +319,7 @@ public final class NumberToTextConverter {
 			sb.append('E');
 			sb.append('-');
 			appendExp(sb, absExponent);
-		} else { 
+		} else {
 			sb.insert(0, "0.");
 			for(int i=numberOfLeadingZeros; i>0; i--) {
 				sb.insert(2, '0');
@@ -345,7 +345,7 @@ public final class NumberToTextConverter {
 			return;
 		}
 		sb.append(val);
-		
+
 	}
 
 
@@ -391,12 +391,12 @@ public final class NumberToTextConverter {
 		while(sb.charAt(changeDigitIx) == '9') {
 			sb.setCharAt(changeDigitIx, '0');
 			changeDigitIx--;
-			// All nines, rounded up.  Notify caller 
+			// All nines, rounded up.  Notify caller
 			if(changeDigitIx < 0) {
 				return true;
 			}
 		}
-		// no more '9's to round up.  
+		// no more '9's to round up.
 		// Last digit to be changed is still inside sb
 		char prevDigit = sb.charAt(changeDigitIx);
 		sb.setCharAt(changeDigitIx, (char) (prevDigit + 1));

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

@@ -22,11 +22,11 @@ import junit.framework.TestSuite;
 
 /**
  * Collects all tests the package <tt>org.apache.poi.hssf.record.formula.eval</tt>.
- * 
+ *
  * @author Josh Micich
  */
 public class AllFormulaEvalTests {
-	
+
 	public static Test suite() {
 		TestSuite result = new TestSuite(AllFormulaEvalTests.class.getName());
 		result.addTestSuite(TestAreaEval.class);
@@ -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)));
+		}
+	}
+}