From 73569e99bd4337328bc86f281a1be98799ced05d Mon Sep 17 00:00:00 2001 From: Shane Carr Date: Wed, 20 Dec 2017 01:41:08 +0000 Subject: [PATCH] ICU-13443 Making MAX_INT_FRAC_SIG checking consistent between inclusive and exclusive ranges. (Changing all comparisons to be inclusive.) X-SVN-Rev: 40746 --- .../com/ibm/icu/number/FractionRounder.java | 8 +- .../src/com/ibm/icu/number/IntegerWidth.java | 12 +- .../core/src/com/ibm/icu/number/Rounder.java | 28 ++--- .../ibm/icu/number/ScientificNotation.java | 4 +- .../test/number/NumberFormatterApiTest.java | 116 ++++++++++++++++++ 5 files changed, 142 insertions(+), 26 deletions(-) diff --git a/icu4j/main/classes/core/src/com/ibm/icu/number/FractionRounder.java b/icu4j/main/classes/core/src/com/ibm/icu/number/FractionRounder.java index 5662e360b82..2ced2a44bad 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/number/FractionRounder.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/number/FractionRounder.java @@ -39,11 +39,11 @@ public abstract class FractionRounder extends Rounder { * @see NumberFormatter */ public Rounder withMinDigits(int minSignificantDigits) { - if (minSignificantDigits > 0 && minSignificantDigits <= RoundingUtils.MAX_INT_FRAC_SIG) { + if (minSignificantDigits >= 1 && minSignificantDigits <= RoundingUtils.MAX_INT_FRAC_SIG) { return constructFractionSignificant(this, minSignificantDigits, -1); } else { throw new IllegalArgumentException( - "Significant digits must be between 0 and " + RoundingUtils.MAX_INT_FRAC_SIG); + "Significant digits must be between 1 and " + RoundingUtils.MAX_INT_FRAC_SIG + " (inclusive)"); } } @@ -67,11 +67,11 @@ public abstract class FractionRounder extends Rounder { * @see NumberFormatter */ public Rounder withMaxDigits(int maxSignificantDigits) { - if (maxSignificantDigits > 0 && maxSignificantDigits <= RoundingUtils.MAX_INT_FRAC_SIG) { + if (maxSignificantDigits >= 1 && maxSignificantDigits <= RoundingUtils.MAX_INT_FRAC_SIG) { return constructFractionSignificant(this, -1, maxSignificantDigits); } else { throw new IllegalArgumentException( - "Significant digits must be between 0 and " + RoundingUtils.MAX_INT_FRAC_SIG); + "Significant digits must be between 1 and " + RoundingUtils.MAX_INT_FRAC_SIG + " (inclusive)"); } } } \ No newline at end of file diff --git a/icu4j/main/classes/core/src/com/ibm/icu/number/IntegerWidth.java b/icu4j/main/classes/core/src/com/ibm/icu/number/IntegerWidth.java index 90fe0dcc391..4d75894fad3 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/number/IntegerWidth.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/number/IntegerWidth.java @@ -42,11 +42,11 @@ public class IntegerWidth { public static IntegerWidth zeroFillTo(int minInt) { if (minInt == 1) { return DEFAULT; - } else if (minInt >= 0 && minInt < RoundingUtils.MAX_INT_FRAC_SIG) { + } else if (minInt >= 0 && minInt <= RoundingUtils.MAX_INT_FRAC_SIG) { return new IntegerWidth(minInt, -1); } else { throw new IllegalArgumentException( - "Integer digits must be between 0 and " + RoundingUtils.MAX_INT_FRAC_SIG); + "Integer digits must be between 0 and " + RoundingUtils.MAX_INT_FRAC_SIG + " (inclusive)"); } } @@ -56,7 +56,7 @@ public class IntegerWidth { * For example, with maxInt=3, the number 1234 will get printed as "234". * * @param maxInt - * The maximum number of places before the decimal separator. + * The maximum number of places before the decimal separator. maxInt == -1 means no truncation. * @return An IntegerWidth for passing to the NumberFormatter integerWidth() setter. * @draft ICU 60 * @provisional This API might change or be removed in a future release. @@ -65,13 +65,13 @@ public class IntegerWidth { public IntegerWidth truncateAt(int maxInt) { if (maxInt == this.maxInt) { return this; - } else if (maxInt >= 0 && maxInt < RoundingUtils.MAX_INT_FRAC_SIG) { + } else if (maxInt >= 0 && maxInt <= RoundingUtils.MAX_INT_FRAC_SIG && maxInt >= minInt) { return new IntegerWidth(minInt, maxInt); } else if (maxInt == -1) { - return new IntegerWidth(minInt, maxInt); + return new IntegerWidth(minInt, -1); } else { throw new IllegalArgumentException( - "Integer digits must be between 0 and " + RoundingUtils.MAX_INT_FRAC_SIG); + "Integer digits must be between -1 and " + RoundingUtils.MAX_INT_FRAC_SIG + " (inclusive)"); } } } \ No newline at end of file diff --git a/icu4j/main/classes/core/src/com/ibm/icu/number/Rounder.java b/icu4j/main/classes/core/src/com/ibm/icu/number/Rounder.java index 5624fa8a5f8..38db8c861fb 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/number/Rounder.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/number/Rounder.java @@ -98,7 +98,7 @@ public abstract class Rounder implements Cloneable { return constructFraction(minMaxFractionPlaces, minMaxFractionPlaces); } else { throw new IllegalArgumentException( - "Fraction length must be between 0 and " + RoundingUtils.MAX_INT_FRAC_SIG); + "Fraction length must be between 0 and " + RoundingUtils.MAX_INT_FRAC_SIG + " (inclusive)"); } } @@ -118,11 +118,11 @@ public abstract class Rounder implements Cloneable { * @see NumberFormatter */ public static FractionRounder minFraction(int minFractionPlaces) { - if (minFractionPlaces >= 0 && minFractionPlaces < RoundingUtils.MAX_INT_FRAC_SIG) { + if (minFractionPlaces >= 0 && minFractionPlaces <= RoundingUtils.MAX_INT_FRAC_SIG) { return constructFraction(minFractionPlaces, -1); } else { throw new IllegalArgumentException( - "Fraction length must be between 0 and " + RoundingUtils.MAX_INT_FRAC_SIG); + "Fraction length must be between 0 and " + RoundingUtils.MAX_INT_FRAC_SIG + " (inclusive)"); } } @@ -139,11 +139,11 @@ public abstract class Rounder implements Cloneable { * @see NumberFormatter */ public static FractionRounder maxFraction(int maxFractionPlaces) { - if (maxFractionPlaces >= 0 && maxFractionPlaces < RoundingUtils.MAX_INT_FRAC_SIG) { + if (maxFractionPlaces >= 0 && maxFractionPlaces <= RoundingUtils.MAX_INT_FRAC_SIG) { return constructFraction(0, maxFractionPlaces); } else { throw new IllegalArgumentException( - "Fraction length must be between 0 and " + RoundingUtils.MAX_INT_FRAC_SIG); + "Fraction length must be between 0 and " + RoundingUtils.MAX_INT_FRAC_SIG + " (inclusive)"); } } @@ -168,7 +168,7 @@ public abstract class Rounder implements Cloneable { return constructFraction(minFractionPlaces, maxFractionPlaces); } else { throw new IllegalArgumentException( - "Fraction length must be between 0 and " + RoundingUtils.MAX_INT_FRAC_SIG); + "Fraction length must be between 0 and " + RoundingUtils.MAX_INT_FRAC_SIG + " (inclusive)"); } } @@ -188,11 +188,11 @@ public abstract class Rounder implements Cloneable { * @see NumberFormatter */ public static Rounder fixedDigits(int minMaxSignificantDigits) { - if (minMaxSignificantDigits > 0 && minMaxSignificantDigits <= RoundingUtils.MAX_INT_FRAC_SIG) { + if (minMaxSignificantDigits >= 1 && minMaxSignificantDigits <= RoundingUtils.MAX_INT_FRAC_SIG) { return constructSignificant(minMaxSignificantDigits, minMaxSignificantDigits); } else { throw new IllegalArgumentException( - "Significant digits must be between 0 and " + RoundingUtils.MAX_INT_FRAC_SIG); + "Significant digits must be between 1 and " + RoundingUtils.MAX_INT_FRAC_SIG + " (inclusive)"); } } @@ -211,11 +211,11 @@ public abstract class Rounder implements Cloneable { * @see NumberFormatter */ public static Rounder minDigits(int minSignificantDigits) { - if (minSignificantDigits > 0 && minSignificantDigits <= RoundingUtils.MAX_INT_FRAC_SIG) { + if (minSignificantDigits >= 1 && minSignificantDigits <= RoundingUtils.MAX_INT_FRAC_SIG) { return constructSignificant(minSignificantDigits, -1); } else { throw new IllegalArgumentException( - "Significant digits must be between 0 and " + RoundingUtils.MAX_INT_FRAC_SIG); + "Significant digits must be between 1 and " + RoundingUtils.MAX_INT_FRAC_SIG + " (inclusive)"); } } @@ -230,11 +230,11 @@ public abstract class Rounder implements Cloneable { * @see NumberFormatter */ public static Rounder maxDigits(int maxSignificantDigits) { - if (maxSignificantDigits > 0 && maxSignificantDigits <= RoundingUtils.MAX_INT_FRAC_SIG) { + if (maxSignificantDigits >= 1 && maxSignificantDigits <= RoundingUtils.MAX_INT_FRAC_SIG) { return constructSignificant(0, maxSignificantDigits); } else { throw new IllegalArgumentException( - "Significant digits must be between 0 and " + RoundingUtils.MAX_INT_FRAC_SIG); + "Significant digits must be between 1 and " + RoundingUtils.MAX_INT_FRAC_SIG + " (inclusive)"); } } @@ -252,12 +252,12 @@ public abstract class Rounder implements Cloneable { * @see NumberFormatter */ public static Rounder minMaxDigits(int minSignificantDigits, int maxSignificantDigits) { - if (minSignificantDigits > 0 && maxSignificantDigits <= RoundingUtils.MAX_INT_FRAC_SIG + if (minSignificantDigits >= 1 && maxSignificantDigits <= RoundingUtils.MAX_INT_FRAC_SIG && minSignificantDigits <= maxSignificantDigits) { return constructSignificant(minSignificantDigits, maxSignificantDigits); } else { throw new IllegalArgumentException( - "Significant digits must be between 0 and " + RoundingUtils.MAX_INT_FRAC_SIG); + "Significant digits must be between 1 and " + RoundingUtils.MAX_INT_FRAC_SIG + " (inclusive)"); } } diff --git a/icu4j/main/classes/core/src/com/ibm/icu/number/ScientificNotation.java b/icu4j/main/classes/core/src/com/ibm/icu/number/ScientificNotation.java index fe0e274d26b..c0f7a60293a 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/number/ScientificNotation.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/number/ScientificNotation.java @@ -55,13 +55,13 @@ public class ScientificNotation extends Notation implements Cloneable { * @see NumberFormatter */ public ScientificNotation withMinExponentDigits(int minExponentDigits) { - if (minExponentDigits >= 0 && minExponentDigits < RoundingUtils.MAX_INT_FRAC_SIG) { + if (minExponentDigits >= 1 && minExponentDigits <= RoundingUtils.MAX_INT_FRAC_SIG) { ScientificNotation other = (ScientificNotation) this.clone(); other.minExponentDigits = minExponentDigits; return other; } else { throw new IllegalArgumentException( - "Integer digits must be between 0 and " + RoundingUtils.MAX_INT_FRAC_SIG); + "Integer digits must be between 1 and " + RoundingUtils.MAX_INT_FRAC_SIG + " (inclusive)"); } } diff --git a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java index 835a5fa320a..af6df82eeac 100644 --- a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java +++ b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java @@ -4,10 +4,17 @@ package com.ibm.icu.dev.test.number; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertTrue; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; import java.math.BigDecimal; import java.math.RoundingMode; +import java.util.HashMap; +import java.util.HashSet; import java.util.Locale; +import java.util.Map; +import java.util.Set; import org.junit.Ignore; import org.junit.Test; @@ -16,6 +23,7 @@ import com.ibm.icu.impl.number.Padder; import com.ibm.icu.impl.number.Padder.PadPosition; import com.ibm.icu.impl.number.PatternStringParser; import com.ibm.icu.number.FormattedNumber; +import com.ibm.icu.number.FractionRounder; import com.ibm.icu.number.Grouper; import com.ibm.icu.number.IntegerWidth; import com.ibm.icu.number.LocalizedNumberFormatter; @@ -25,6 +33,7 @@ import com.ibm.icu.number.NumberFormatter.DecimalSeparatorDisplay; import com.ibm.icu.number.NumberFormatter.SignDisplay; import com.ibm.icu.number.NumberFormatter.UnitWidth; import com.ibm.icu.number.Rounder; +import com.ibm.icu.number.ScientificNotation; import com.ibm.icu.number.UnlocalizedNumberFormatter; import com.ibm.icu.text.DecimalFormatSymbols; import com.ibm.icu.text.NumberingSystem; @@ -1560,6 +1569,113 @@ public class NumberFormatterApiTest { "1.00 US dollars"); } + @Test + public void validRanges() throws NoSuchMethodException, IllegalAccessException { + Method[] methodsWithOneArgument = new Method[] { Rounder.class.getDeclaredMethod("fixedFraction", Integer.TYPE), + Rounder.class.getDeclaredMethod("minFraction", Integer.TYPE), + Rounder.class.getDeclaredMethod("maxFraction", Integer.TYPE), + Rounder.class.getDeclaredMethod("fixedDigits", Integer.TYPE), + Rounder.class.getDeclaredMethod("minDigits", Integer.TYPE), + Rounder.class.getDeclaredMethod("maxDigits", Integer.TYPE), + FractionRounder.class.getDeclaredMethod("withMinDigits", Integer.TYPE), + FractionRounder.class.getDeclaredMethod("withMaxDigits", Integer.TYPE), + ScientificNotation.class.getDeclaredMethod("withMinExponentDigits", Integer.TYPE), + IntegerWidth.class.getDeclaredMethod("zeroFillTo", Integer.TYPE), + IntegerWidth.class.getDeclaredMethod("truncateAt", Integer.TYPE), }; + Method[] methodsWithTwoArguments = new Method[] { + Rounder.class.getDeclaredMethod("minMaxFraction", Integer.TYPE, Integer.TYPE), + Rounder.class.getDeclaredMethod("minMaxDigits", Integer.TYPE, Integer.TYPE), }; + + final int EXPECTED_MAX_INT_FRAC_SIG = 100; + final String expectedSubstring0 = "between 0 and 100 (inclusive)"; + final String expectedSubstring1 = "between 1 and 100 (inclusive)"; + final String expectedSubstringN1 = "between -1 and 100 (inclusive)"; + + // We require that the upper bounds all be 100 inclusive. + // The lower bound may be either -1, 0, or 1. + Set methodsWithLowerBound1 = new HashSet(); + methodsWithLowerBound1.add("fixedDigits"); + methodsWithLowerBound1.add("minDigits"); + methodsWithLowerBound1.add("maxDigits"); + methodsWithLowerBound1.add("minMaxDigits"); + methodsWithLowerBound1.add("withMinDigits"); + methodsWithLowerBound1.add("withMaxDigits"); + methodsWithLowerBound1.add("withMinExponentDigits"); + Set methodsWithLowerBoundN1 = new HashSet(); + methodsWithLowerBoundN1.add("truncateAt"); + + // Some of the methods require an object to be called upon. + Map targets = new HashMap(); + targets.put("withMinDigits", Rounder.integer()); + targets.put("withMaxDigits", Rounder.integer()); + targets.put("withMinExponentDigits", Notation.scientific()); + targets.put("truncateAt", IntegerWidth.zeroFillTo(0)); + + for (int argument = -2; argument <= EXPECTED_MAX_INT_FRAC_SIG + 2; argument++) { + for (Method method : methodsWithOneArgument) { + String message = "i = " + argument + "; method = " + method.getName(); + int lowerBound = methodsWithLowerBound1.contains(method.getName()) ? 1 + : methodsWithLowerBoundN1.contains(method.getName()) ? -1 : 0; + String expectedSubstring = lowerBound == 0 ? expectedSubstring0 + : lowerBound == 1 ? expectedSubstring1 : expectedSubstringN1; + Object target = targets.get(method.getName()); + try { + method.invoke(target, argument); + assertTrue(message, argument >= lowerBound && argument <= EXPECTED_MAX_INT_FRAC_SIG); + } catch (InvocationTargetException e) { + assertTrue(message, argument < lowerBound || argument > EXPECTED_MAX_INT_FRAC_SIG); + // Ensure the exception message contains the expected substring + String actualMessage = e.getCause().getMessage(); + assertNotEquals(message + ": " + actualMessage, -1, actualMessage.indexOf(expectedSubstring)); + } + } + for (Method method : methodsWithTwoArguments) { + String message = "i = " + argument + "; method = " + method.getName(); + int lowerBound = methodsWithLowerBound1.contains(method.getName()) ? 1 + : methodsWithLowerBoundN1.contains(method.getName()) ? -1 : 0; + String expectedSubstring = lowerBound == 0 ? expectedSubstring0 : expectedSubstring1; + Object target = targets.get(method.getName()); + // Check range on the first argument + try { + // Pass EXPECTED_MAX_INT_FRAC_SIG as the second argument so arg1 <= arg2 in expected cases + method.invoke(target, argument, EXPECTED_MAX_INT_FRAC_SIG); + assertTrue(message, argument >= lowerBound && argument <= EXPECTED_MAX_INT_FRAC_SIG); + } catch (InvocationTargetException e) { + assertTrue(message, argument < lowerBound || argument > EXPECTED_MAX_INT_FRAC_SIG); + // Ensure the exception message contains the expected substring + String actualMessage = e.getCause().getMessage(); + assertNotEquals(message + ": " + actualMessage, -1, actualMessage.indexOf(expectedSubstring)); + } + // Check range on the second argument + try { + // Pass lowerBound as the first argument so arg1 <= arg2 in expected cases + method.invoke(target, lowerBound, argument); + assertTrue(message, argument >= lowerBound && argument <= EXPECTED_MAX_INT_FRAC_SIG); + } catch (InvocationTargetException e) { + assertTrue(message, argument < lowerBound || argument > EXPECTED_MAX_INT_FRAC_SIG); + // Ensure the exception message contains the expected substring + String actualMessage = e.getCause().getMessage(); + assertNotEquals(message + ": " + actualMessage, -1, actualMessage.indexOf(expectedSubstring)); + } + // Check that first argument must be less than or equal to second argument + try { + method.invoke(target, argument, argument - 1); + org.junit.Assert.fail(); + } catch (InvocationTargetException e) { + // Pass + } + } + } + + // Check first argument less than or equal to second argument on IntegerWidth + try { + IntegerWidth.zeroFillTo(4).truncateAt(2); + org.junit.Assert.fail(); + } catch (IllegalArgumentException e) { + // Pass + } + } + private static void assertFormatDescending( String message, String skeleton, -- 2.40.0