Repository: incubator-freemarker Updated Branches: refs/heads/3 0b02af638 -> 2385a24d4
Forward ported from 2.3-gae: The default arithmetic engine (ArithmeticEngine.BIGDECIMAL_ENGINE) can now compare infinite (both positive and negative) to any other standard type. Earlier, since BigDecimal can't represent infinite, it was only working in certain special cases. Also did some performance optimizations to slightly decrease the impact and number of conversions to BigDecimal. Project: http://git-wip-us.apache.org/repos/asf/incubator-freemarker/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-freemarker/commit/2385a24d Tree: http://git-wip-us.apache.org/repos/asf/incubator-freemarker/tree/2385a24d Diff: http://git-wip-us.apache.org/repos/asf/incubator-freemarker/diff/2385a24d Branch: refs/heads/3 Commit: 2385a24d498a66cdab820aba69489dd1bedc5204 Parents: 0b02af6 Author: ddekany <ddek...@apache.org> Authored: Sun Mar 11 22:23:39 2018 +0100 Committer: ddekany <ddek...@apache.org> Committed: Sun Mar 11 22:23:39 2018 +0100 ---------------------------------------------------------------------- .../impl/BigDecimalArithmeticEngineTest.java | 97 ++++++++++++++++++++ .../impl/BigDecimalArithmeticEngine.java | 84 ++++++++++++++++- .../freemarker/core/util/_NumberUtils.java | 36 +++++++- 3 files changed, 208 insertions(+), 9 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/2385a24d/freemarker-core-test/src/test/java/org/apache/freemarker/core/arithmetic/impl/BigDecimalArithmeticEngineTest.java ---------------------------------------------------------------------- diff --git a/freemarker-core-test/src/test/java/org/apache/freemarker/core/arithmetic/impl/BigDecimalArithmeticEngineTest.java b/freemarker-core-test/src/test/java/org/apache/freemarker/core/arithmetic/impl/BigDecimalArithmeticEngineTest.java new file mode 100644 index 0000000..422ec16 --- /dev/null +++ b/freemarker-core-test/src/test/java/org/apache/freemarker/core/arithmetic/impl/BigDecimalArithmeticEngineTest.java @@ -0,0 +1,97 @@ +/* + * 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.freemarker.core.arithmetic.impl; + +import static org.apache.freemarker.core.arithmetic.impl.BigDecimalArithmeticEngine.*; +import static org.junit.Assert.*; + +import java.math.BigDecimal; +import java.math.BigInteger; + +import org.junit.Test; + +public class BigDecimalArithmeticEngineTest { + + @Test + public void compareNumbersZeroTest() { + assertEquals(0, INSTANCE.compareNumbers(0, new BigDecimal("-0"))); + assertEquals(0, INSTANCE.compareNumbers(0.0, new BigDecimal("-0"))); + assertEquals(0, INSTANCE.compareNumbers(-0.0, new BigDecimal("+0"))); + assertEquals(0, INSTANCE.compareNumbers(-0.0, 0.0)); + } + + @Test + public void compareNumbersNoRoundingGlitchTest() { + assertEquals(0, INSTANCE.compareNumbers(1.1, new BigDecimal("1.1"))); + assertEquals(-1, INSTANCE.compareNumbers(Double.MIN_VALUE, Math.nextUp(Double.MIN_VALUE))); + assertEquals(-1, INSTANCE.compareNumbers( + Double.MIN_VALUE, new BigDecimal("" + Math.nextUp(Double.MIN_VALUE)))); + } + + @Test + public void compareNumbersSameTypeTest() { + assertEquals(-1, INSTANCE.compareNumbers(1, 2)); + assertEquals(-1, INSTANCE.compareNumbers(1L, 2L)); + assertEquals(-1, INSTANCE.compareNumbers((short) 1, (short) 2)); + assertEquals(-1, INSTANCE.compareNumbers((byte) 1, (byte) 2)); + assertEquals(-1, INSTANCE.compareNumbers(1.0, 2.0)); + assertEquals(-1, INSTANCE.compareNumbers(1.0f, 2.0f)); + assertEquals(-1, INSTANCE.compareNumbers(BigDecimal.ONE, BigDecimal.TEN)); + } + + @Test + public void compareNumbersScaleDoesNotMatterTest() { + assertEquals(0, INSTANCE.compareNumbers(1.0, new BigDecimal("1"))); + assertEquals(0, INSTANCE.compareNumbers(1.0, new BigDecimal("1.00"))); + assertEquals(0, INSTANCE.compareNumbers(1.0f, new BigDecimal("0.1E1"))); + assertEquals(0, INSTANCE.compareNumbers(1, new BigDecimal("1.0"))); + } + + @Test + public void compareNumbersInfinityTest() { + for (boolean isFloat : new boolean[] { false, true }) { + Number negInf = isFloat ? Float.NEGATIVE_INFINITY : Double.NEGATIVE_INFINITY; + Number posInf = isFloat ? Float.POSITIVE_INFINITY : Double.POSITIVE_INFINITY; + + assertEquals(-1, INSTANCE.compareNumbers(negInf, posInf)); + assertEquals(1, INSTANCE.compareNumbers(posInf, negInf)); + assertEquals(0, INSTANCE.compareNumbers(posInf, posInf)); + assertEquals(0, INSTANCE.compareNumbers(negInf, negInf)); + + Number otherNegInf = !isFloat ? Float.NEGATIVE_INFINITY : Double.NEGATIVE_INFINITY; + Number otherPosInf = !isFloat ? Float.POSITIVE_INFINITY : Double.POSITIVE_INFINITY; + assertEquals(-1, INSTANCE.compareNumbers(negInf, otherPosInf)); + assertEquals(1, INSTANCE.compareNumbers(posInf, otherNegInf)); + assertEquals(0, INSTANCE.compareNumbers(posInf, otherPosInf)); + assertEquals(0, INSTANCE.compareNumbers(negInf, otherNegInf)); + + for (Number one : new Number[] { + BigDecimal.ONE, 1.0, 1f, 1, 1L, (byte) 1, (short) 1, BigInteger.ONE, + BigDecimal.ONE.negate(), -1.0, -1f, -1, -1L, (byte) -1, (short) -1, BigInteger.ONE.negate(), + BigDecimal.ZERO, 0.0, 0f, 0, 0L, (byte) 0, (short) 0, BigInteger.ZERO }) { + assertEquals(-1, INSTANCE.compareNumbers(negInf, one)); + assertEquals(1, INSTANCE.compareNumbers(posInf, one)); + assertEquals(1, INSTANCE.compareNumbers(one, negInf)); + assertEquals(-1, INSTANCE.compareNumbers(one, posInf)); + } + } + } + +} http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/2385a24d/freemarker-core/src/main/java/org/apache/freemarker/core/arithmetic/impl/BigDecimalArithmeticEngine.java ---------------------------------------------------------------------- diff --git a/freemarker-core/src/main/java/org/apache/freemarker/core/arithmetic/impl/BigDecimalArithmeticEngine.java b/freemarker-core/src/main/java/org/apache/freemarker/core/arithmetic/impl/BigDecimalArithmeticEngine.java index d09d48f..827b0e8 100644 --- a/freemarker-core/src/main/java/org/apache/freemarker/core/arithmetic/impl/BigDecimalArithmeticEngine.java +++ b/freemarker-core/src/main/java/org/apache/freemarker/core/arithmetic/impl/BigDecimalArithmeticEngine.java @@ -40,16 +40,90 @@ public class BigDecimalArithmeticEngine extends ArithmeticEngine { // We try to find the result based on the sign (+/-/0) first, because: // - It's much faster than converting to BigDecial, and comparing to 0 is the most common comparison. // - It doesn't require any type conversions, and thus things like "Infinity > 0" won't fail. - int firstSignum = _NumberUtils.getSignum(first); + int firstSignum = _NumberUtils.getSignum(first); int secondSignum = _NumberUtils.getSignum(second); if (firstSignum != secondSignum) { - return firstSignum < secondSignum ? -1 : (firstSignum > secondSignum ? 1 : 0); + return firstSignum < secondSignum ? -1 : (firstSignum > secondSignum ? 1 : 0); } else if (firstSignum == 0 && secondSignum == 0) { return 0; } else { - BigDecimal left = _NumberUtils.toBigDecimal(first); - BigDecimal right = _NumberUtils.toBigDecimal(second); - return left.compareTo(right); + // The most common case is comparing values of the same type. As BigDecimal can represent all of these + // with loseless round-trip (i.e., converting to BigDecimal and then back the original type gives the + // original value), we can avoid conversion to BigDecimal without changing the result. + if (first.getClass() == second.getClass()) { + // Bit of optimization for this is a very common case: + if (first instanceof BigDecimal) { + return ((BigDecimal) first).compareTo((BigDecimal) second); + } + + if (first instanceof Integer) { + return ((Integer) first).compareTo((Integer) second); + } + if (first instanceof Long) { + return ((Long) first).compareTo((Long) second); + } + if (first instanceof Double) { + return ((Double) first).compareTo((Double) second); + } + if (first instanceof Float) { + return ((Float) first).compareTo((Float) second); + } + if (first instanceof Byte) { + return ((Byte) first).compareTo((Byte) second); + } + if (first instanceof Short) { + return ((Short) first).compareTo((Short) second); + } + } + // We are going to compare values of two different types. + + // Handle infinity before we try conversion to BigDecimal, as that BigDecimal can't represent that: + if (first instanceof Double) { + double firstD = first.doubleValue(); + if (Double.isInfinite(firstD)) { + if (_NumberUtils.hasTypeThatIsKnownToNotSupportInfiniteAndNaN(second)) { + return firstD == Double.NEGATIVE_INFINITY ? -1 : 1; + } + if (second instanceof Float) { + return Double.compare(firstD, second.doubleValue()); + } + } + } + if (first instanceof Float) { + float firstF = first.floatValue(); + if (Float.isInfinite(firstF)) { + if (_NumberUtils.hasTypeThatIsKnownToNotSupportInfiniteAndNaN(second)) { + return firstF == Float.NEGATIVE_INFINITY ? -1 : 1; + } + if (second instanceof Double) { + return Double.compare(firstF, second.doubleValue()); + } + } + } + if (second instanceof Double) { + double secondD = second.doubleValue(); + if (Double.isInfinite(secondD)) { + if (_NumberUtils.hasTypeThatIsKnownToNotSupportInfiniteAndNaN(first)) { + return secondD == Double.NEGATIVE_INFINITY ? 1 : -1; + } + if (first instanceof Float) { + return Double.compare(first.doubleValue(), secondD); + } + } + } + if (second instanceof Float) { + float secondF = second.floatValue(); + if (Float.isInfinite(secondF)) { + if (_NumberUtils.hasTypeThatIsKnownToNotSupportInfiniteAndNaN(first)) { + return secondF == Float.NEGATIVE_INFINITY ? 1 : -1; + } + if (first instanceof Double) { + return Double.compare(first.doubleValue(), secondF); + } + } + } + + return _NumberUtils.toBigDecimal(first).compareTo(_NumberUtils.toBigDecimal(second)); } } http://git-wip-us.apache.org/repos/asf/incubator-freemarker/blob/2385a24d/freemarker-core/src/main/java/org/apache/freemarker/core/util/_NumberUtils.java ---------------------------------------------------------------------- diff --git a/freemarker-core/src/main/java/org/apache/freemarker/core/util/_NumberUtils.java b/freemarker-core/src/main/java/org/apache/freemarker/core/util/_NumberUtils.java index 5cff40b..9c65eaf 100644 --- a/freemarker-core/src/main/java/org/apache/freemarker/core/util/_NumberUtils.java +++ b/freemarker-core/src/main/java/org/apache/freemarker/core/util/_NumberUtils.java @@ -39,7 +39,7 @@ public class _NumberUtils { return ((Double) num).isInfinite(); } else if (num instanceof Float) { return ((Float) num).isInfinite(); - } else if (isNonFPNumberOfSupportedClass(num)) { + } else if (hasTypeThatIsKnownToNotSupportInfiniteAndNaN(num)) { return false; } else { throw new UnsupportedNumberClassException(num.getClass()); @@ -51,7 +51,7 @@ public class _NumberUtils { return ((Double) num).isNaN(); } else if (num instanceof Float) { return ((Float) num).isNaN(); - } else if (isNonFPNumberOfSupportedClass(num)) { + } else if (hasTypeThatIsKnownToNotSupportInfiniteAndNaN(num)) { return false; } else { throw new UnsupportedNumberClassException(num.getClass()); @@ -110,7 +110,12 @@ public class _NumberUtils { // condition, but stripTrailingZeros was slower than setScale + compareTo. } - private static boolean isNonFPNumberOfSupportedClass(Number num) { + /** + * Tells if the type of the parameter number is known to not be able to represent infinite (positive or negative) + * and NaN. If this returns {@code false}, that doesn't mean that it can do that, because it's maybe just that this + * utility doesn't know that type. + */ + public static boolean hasTypeThatIsKnownToNotSupportInfiniteAndNaN(Number num) { return num instanceof Integer || num instanceof BigDecimal || num instanceof Long || num instanceof Short || num instanceof Byte || num instanceof BigInteger; } @@ -199,10 +204,33 @@ public class _NumberUtils { return number; } + /** + * Convert a {@code Number} to {@link BigDecimal}. + * + * @throws NumberFormatException + * If the conversion is not possible, e.g. Infinite and NaN can't be converted to {@link BigDecimal}. + */ public static BigDecimal toBigDecimal(Number num) { + if (num instanceof BigDecimal) { + return (BigDecimal) num; + } + if (num instanceof Integer || num instanceof Long || num instanceof Byte || num instanceof Short) { + return BigDecimal.valueOf(num.longValue()); + } + if (num instanceof BigInteger) { + return new BigDecimal((BigInteger) num); + } try { - return num instanceof BigDecimal ? (BigDecimal) num : new BigDecimal(num.toString()); + // Why toString? It's partly for backward compatibility. But it's also better for Double (and Float) values + // than new BigDecimal(someDouble), which is overly precise. For example, if you have `double d = 0.1`, then + // `new BigDecimal(d).equals(new BigDecimal("0.1"))` is `false`, while + // `new BigDecimal(Double.toString(d)).equals(new BigDecimal("0.1"))` is `true`. + return new BigDecimal(num.toString()); } catch (NumberFormatException e) { + if (isInfinite(num)) { + throw new NumberFormatException("It's impossible to convert an infinte value (" + + num.getClass().getSimpleName() + " " + num + ") to BigDecimal."); + } // The exception message is useless, so we add a new one: throw new NumberFormatException("Can't parse this as BigDecimal number: " + _StringUtils.jQuote(num)); }
