organicmaps/icu
13
Fork 0
mirror of https://github.com/unicode-org/icu.git synced 2025-04-13 00:43:32 +00:00
Code Issues 1 Projects Releases Wiki Activity Actions 3

ICU-13634 DecimalQuantity work: Fixing integer overflow behavior in toLong and toFractionLong methods. Adding test for maxInt/maxFrac behavior in toScientificString and related methods. Updating a few test expectations in IntlTestDecimalFormatAPI::TestFixedDecimal, which is now passing.

Browse source 
X-SVN-Rev: 41235
This commit is contained in:
Shane Carr 2018-04-17 01:36:18 +00:00
parent f84f0b726e
commit d6c6fa0404
12 changed files with 147 additions and 91 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

77
icu4c/source/i18n/number_decimalquantity.cpp
View file

@ -168,8 +168,9 @@ uint64_t DecimalQuantity::getPositionFingerprint() const {
void DecimalQuantity::roundToIncrement(double roundingIncrement, RoundingMode roundingMode,
int32_t maxFrac, UErrorCode& status) {
// TODO: This is innefficient. Improve?
// TODO: Should we convert to decNumber instead?
// TODO(13701): This is innefficient. Improve?
// TODO(13701): Should we convert to decNumber instead?
roundToInfinity();
double temp = toDouble();
temp /= roundingIncrement;
setToDouble(temp);
@ -246,7 +247,7 @@ double DecimalQuantity::getPluralOperand(PluralOperand operand) const {
switch (operand) {
case PLURAL_OPERAND_I:
// Invert the negative sign if necessary
return static_cast<double>(isNegative() ? -toLong() : toLong());
return static_cast<double>(isNegative() ? -toLong(true) : toLong(true));
case PLURAL_OPERAND_F:
return static_cast<double>(toFractionLong(true));
case PLURAL_OPERAND_T:
@ -260,6 +261,10 @@ double DecimalQuantity::getPluralOperand(PluralOperand operand) const {
}
}
bool DecimalQuantity::hasIntegerValue() const {
return scale >= 0;
}
int32_t DecimalQuantity::getUpperDisplayMagnitude() const {
// If this assertion fails, you need to call roundToInfinity() or some other rounding method.
// See the comment in the header file explaining the "isApproximate" field.
@ -489,11 +494,17 @@ void DecimalQuantity::_setToDecNum(const DecNum& decnum, UErrorCode& status) {
}
}
int64_t DecimalQuantity::toLong() const {
int64_t DecimalQuantity::toLong(bool truncateIfOverflow) const {
// NOTE: Call sites should be guarded by fitsInLong(), like this:
// if (dq.fitsInLong()) { /* use dq.toLong() */ } else { /* use some fallback */ }
// Fallback behavior upon truncateIfOverflow is to truncate at 17 digits.
U_ASSERT(truncateIfOverflow || fitsInLong());
int64_t result = 0L;
for (int32_t magnitude = scale + precision - 1; magnitude >= 0; magnitude--) {
int32_t upperMagnitude = std::min(scale + precision, lOptPos) - 1;
if (truncateIfOverflow) {
upperMagnitude = std::min(upperMagnitude, 17);
}
for (int32_t magnitude = upperMagnitude; magnitude >= 0; magnitude--) {
result = result * 10 + getDigitPos(magnitude - scale);
}
if (isNegative()) {
@ -502,13 +513,22 @@ int64_t DecimalQuantity::toLong() const {
return result;
}
int64_t DecimalQuantity::toFractionLong(bool includeTrailingZeros) const {
int64_t result = 0L;
uint64_t DecimalQuantity::toFractionLong(bool includeTrailingZeros) const {
uint64_t result = 0L;
int32_t magnitude = -1;
for (; (magnitude >= scale || (includeTrailingZeros && magnitude >= rReqPos)) &&
magnitude >= rOptPos; magnitude--) {
int32_t lowerMagnitude = std::max(scale, rOptPos);
if (includeTrailingZeros) {
lowerMagnitude = std::min(lowerMagnitude, rReqPos);
}
for (; magnitude >= lowerMagnitude && result <= 1e18L; magnitude--) {
result = result * 10 + getDigitPos(magnitude - scale);
}
// Remove trailing zeros; this can happen during integer overflow cases.
if (!includeTrailingZeros) {
while (result > 0 && (result % 10) == 0) {
result /= 10;
}
}
return result;
}
@ -542,9 +562,9 @@ bool DecimalQuantity::fitsInLong() const {
}
double DecimalQuantity::toDouble() const {
if (isApproximate) {
return toDoubleFromOriginal();
}
// If this assertion fails, you need to call roundToInfinity() or some other rounding method.
// See the comment in the header file explaining the "isApproximate" field.
U_ASSERT(!isApproximate);
if (isNaN()) {
return NAN;
@ -562,24 +582,6 @@ double DecimalQuantity::toDouble() const {
&count);
}
double DecimalQuantity::toDoubleFromOriginal() const {
double result = origDouble;
int32_t delta = origDelta;
if (delta >= 0) {
// 1e22 is the largest exact double.
for (; delta >= 22; delta -= 22) result *= 1e22;
result *= DOUBLE_MULTIPLIERS[delta];
} else {
// 1e22 is the largest exact double.
for (; delta <= -22; delta += 22) result /= 1e22;
result /= DOUBLE_MULTIPLIERS[-delta];
}
if (isNegative()) {
result = -result;
}
return result;
}
void DecimalQuantity::toDecNum(DecNum& output, UErrorCode& status) const {
// Special handling for zero
if (precision == 0) {
@ -795,15 +797,20 @@ UnicodeString DecimalQuantity::toScientificString() const {
result.append(u"0E+0", -1);
return result;
}
result.append(u'0' + getDigitPos(precision - 1));
if (precision > 1) {
// NOTE: It is not safe to add to lOptPos (aka maxInt) or subtract from
// rOptPos (aka -maxFrac) due to overflow.
int32_t upperPos = std::min(precision + scale, lOptPos) - scale - 1;
int32_t lowerPos = std::max(scale, rOptPos) - scale;
int32_t p = upperPos;
result.append(u'0' + getDigitPos(p));
if ((--p) >= lowerPos) {
result.append(u'.');
for (int32_t i = 1; i < precision; i++) {
result.append(u'0' + getDigitPos(precision - i - 1));
for (; p >= lowerPos; p--) {
result.append(u'0' + getDigitPos(p));
}
}
result.append(u'E');
int32_t _scale = scale + precision - 1;
int32_t _scale = upperPos + scale;
if (_scale < 0) {
_scale *= -1;
result.append(u'-');

9
icu4c/source/i18n/number_decimalquantity.h
View file

@ -143,9 +143,10 @@ class U_I18N_API DecimalQuantity : public IFixedDecimal, public UMemory {
/** @return Whether the value represented by this {@link DecimalQuantity} is not a number. */
bool isNaN() const U_OVERRIDE;
int64_t toLong() const;
/** @param truncateIfOverflow if false and the number does NOT fit, fails with an assertion error. */
int64_t toLong(bool truncateIfOverflow = false) const;
int64_t toFractionLong(bool includeTrailingZeros) const;
uint64_t toFractionLong(bool includeTrailingZeros) const;
/**
* Returns whether or not a Long can fully represent the value stored in this DecimalQuantity.
@ -200,6 +201,8 @@ class U_I18N_API DecimalQuantity : public IFixedDecimal, public UMemory {
double getPluralOperand(PluralOperand operand) const U_OVERRIDE;
bool hasIntegerValue() const U_OVERRIDE;
/**
* Gets the digit at the specified magnitude. For example, if the represented number is 12.3,
* getDigit(-1) returns 3, since 3 is the digit corresponding to 10^-1.
@ -466,8 +469,6 @@ class U_I18N_API DecimalQuantity : public IFixedDecimal, public UMemory {
void convertToAccurateDouble();
double toDoubleFromOriginal() const;
/** Ensure that a byte array of at least 40 digits is allocated. */
void ensureCapacity();

10
icu4c/source/i18n/plurrule.cpp
View file

@ -1480,7 +1480,7 @@ FixedDecimal::FixedDecimal(const FixedDecimal &other) {
decimalDigits = other.decimalDigits;
decimalDigitsWithoutTrailingZeros = other.decimalDigitsWithoutTrailingZeros;
intValue = other.intValue;
hasIntegerValue = other.hasIntegerValue;
_hasIntegerValue = other._hasIntegerValue;
isNegative = other.isNegative;
_isNaN = other._isNaN;
_isInfinite = other._isInfinite;
@ -1504,10 +1504,10 @@ void FixedDecimal::init(double n, int32_t v, int64_t f) {
v = 0;
f = 0;
intValue = 0;
hasIntegerValue = FALSE;
_hasIntegerValue = FALSE;
} else {
intValue = (int64_t)source;
hasIntegerValue = (source == intValue);
_hasIntegerValue = (source == intValue);
}
visibleDecimalDigitCount = v;
@ -1641,6 +1641,10 @@ bool FixedDecimal::isInfinite() const {
return _isInfinite;
}
bool FixedDecimal::hasIntegerValue() const {
return _hasIntegerValue;
}
bool FixedDecimal::isNanOrInfinity() const {
return _isNaN || _isInfinite;
}

8
icu4c/source/i18n/plurrule_impl.h
View file

@ -249,9 +249,8 @@ class U_I18N_API IFixedDecimal {
virtual bool isInfinite() const = 0;
virtual bool hasIntegerValue() {
return getPluralOperand(PLURAL_OPERAND_N) == getPluralOperand(PLURAL_OPERAND_I);
}
/** Whether the number has no nonzero fraction digits. */
virtual bool hasIntegerValue() const = 0;
};
/**
@ -279,6 +278,7 @@ class U_I18N_API FixedDecimal: public IFixedDecimal, public UObject {
double getPluralOperand(PluralOperand operand) const U_OVERRIDE;
bool isNaN() const U_OVERRIDE;
bool isInfinite() const U_OVERRIDE;
bool hasIntegerValue() const U_OVERRIDE;
bool isNanOrInfinity() const; // used in decimfmtimpl.cpp
@ -297,7 +297,7 @@ class U_I18N_API FixedDecimal: public IFixedDecimal, public UObject {
int64_t decimalDigits;
int64_t decimalDigitsWithoutTrailingZeros;
int64_t intValue;
UBool hasIntegerValue;
UBool _hasIntegerValue;
UBool isNegative;
UBool _isNaN;
UBool _isInfinite;

10
icu4c/source/test/intltest/dcfmapts.cpp
View file

@ -825,12 +825,12 @@ void IntlTestDecimalFormatAPI::TestFixedDecimal() {
ASSERT_EQUAL(FALSE, fd.hasIntegerValue());
ASSERT_EQUAL(FALSE, fd.isNegative());
fable.setDecimalNumber("12.345678901234567890123456789", status);
fable.setDecimalNumber("12.3456789012345678900123456789", status);
TEST_ASSERT_STATUS(status);
df->formatToDecimalQuantity(fable, fd, status);
TEST_ASSERT_STATUS(status);
ASSERT_EQUAL(22, fd.getPluralOperand(PLURAL_OPERAND_V));
ASSERT_EQUAL(345678901234567890LL, fd.getPluralOperand(PLURAL_OPERAND_F));
ASSERT_EQUAL(3456789012345678900LL, fd.getPluralOperand(PLURAL_OPERAND_F));
ASSERT_EQUAL(34567890123456789LL, fd.getPluralOperand(PLURAL_OPERAND_T));
ASSERT_EQUAL(12, fd.getPluralOperand(PLURAL_OPERAND_I));
ASSERT_EQUAL(FALSE, fd.hasIntegerValue());
@ -842,8 +842,8 @@ void IntlTestDecimalFormatAPI::TestFixedDecimal() {
df->formatToDecimalQuantity(fable, fd, status);
TEST_ASSERT_STATUS(status);
ASSERT_EQUAL(22, fd.getPluralOperand(PLURAL_OPERAND_V));
ASSERT_EQUAL(123456789012345678LL, fd.getPluralOperand(PLURAL_OPERAND_F));
ASSERT_EQUAL(123456789012345678LL, fd.getPluralOperand(PLURAL_OPERAND_T));
ASSERT_EQUAL(1234567890123456789LL, fd.getPluralOperand(PLURAL_OPERAND_F));
ASSERT_EQUAL(1234567890123456789LL, fd.getPluralOperand(PLURAL_OPERAND_T));
ASSERT_EQUAL(345678901234567890LL, fd.getPluralOperand(PLURAL_OPERAND_I));
ASSERT_EQUAL(FALSE, fd.hasIntegerValue());
ASSERT_EQUAL(FALSE, fd.isNegative());
@ -892,7 +892,7 @@ void IntlTestDecimalFormatAPI::TestFixedDecimal() {
ASSERT_EQUAL(2, fd.getPluralOperand(PLURAL_OPERAND_V));
ASSERT_EQUAL(30, fd.getPluralOperand(PLURAL_OPERAND_F));
ASSERT_EQUAL(3, fd.getPluralOperand(PLURAL_OPERAND_T));
ASSERT_EQUAL(100000000000000000LL, fd.getPluralOperand(PLURAL_OPERAND_I));
ASSERT_EQUAL(0, fd.getPluralOperand(PLURAL_OPERAND_I));
ASSERT_EQUAL(FALSE, fd.hasIntegerValue());
ASSERT_EQUAL(FALSE, fd.isNegative());

1
icu4c/source/test/intltest/numbertest.h
View file

@ -122,6 +122,7 @@ class DecimalQuantityTest : public IntlTest {
void testUseApproximateDoubleWhenAble();
void testHardDoubleConversion();
void testToDouble();
void testMaxDigits();
void runIndexedTest(int32_t index, UBool exec, const char *&name, char *par = 0);

27
icu4c/source/test/intltest/numbertest_decimalquantity.cpp
View file

@ -8,6 +8,7 @@
#include "number_decimalquantity.h"
#include "math.h"
#include <cmath>
#include "number_utils.h"
#include "numbertest.h"
void DecimalQuantityTest::runIndexedTest(int32_t index, UBool exec, const char *&name, char *) {
@ -23,6 +24,7 @@ void DecimalQuantityTest::runIndexedTest(int32_t index, UBool exec, const char *
TESTCASE_AUTO(testUseApproximateDoubleWhenAble);
TESTCASE_AUTO(testHardDoubleConversion);
TESTCASE_AUTO(testToDouble);
TESTCASE_AUTO(testMaxDigits);
TESTCASE_AUTO_END;
}
@ -60,9 +62,6 @@ void DecimalQuantityTest::checkDoubleBehavior(double d, bool explicitRequired) {
assertTrue("Should be using approximate double", !fq.isExplicitExactDouble());
}
UnicodeString baseStr = fq.toString();
assertDoubleEquals(
UnicodeString(u"Initial construction from hard double: ") + baseStr,
d, fq.toDouble());
fq.roundToInfinity();
UnicodeString newStr = fq.toString();
if (explicitRequired) {
@ -358,4 +357,26 @@ void DecimalQuantityTest::testToDouble() {
}
}
void DecimalQuantityTest::testMaxDigits() {
IcuTestErrorCode status(*this, "testMaxDigits");
DecimalQuantity dq;
dq.setToDouble(876.543);
dq.roundToInfinity();
dq.setIntegerLength(0, 2);
dq.setFractionLength(0, 2);
assertEquals("Should trim, toPlainString", "76.54", dq.toPlainString());
assertEquals("Should trim, toScientificString", "7.654E+1", dq.toScientificString());
assertEquals("Should trim, toLong", 76L, dq.toLong());
assertEquals("Should trim, toFractionLong", 54L, dq.toFractionLong(false));
assertEquals("Should trim, toDouble", 76.54, dq.toDouble());
// To test DecNum output, check the round-trip.
DecNum dn;
dq.toDecNum(dn, status);
DecimalQuantity copy;
copy.setToDecNum(dn, status);
if (!logKnownIssue("13701")) {
assertEquals("Should trim, toDecNum", "76.54", copy.toPlainString());
}
}
#endif /* #if !UCONFIG_NO_FORMATTING */

74
icu4j/main/classes/core/src/com/ibm/icu/impl/number/DecimalQuantity_AbstractBCD.java
View file

@ -242,7 +242,7 @@ public abstract class DecimalQuantity_AbstractBCD implements DecimalQuantity {
switch (operand) {
case i:
// Invert the negative sign if necessary
return isNegative() ? -toLong() : toLong();
return isNegative() ? -toLong(true) : toLong(true);
case f:
return toFractionLong(true);
case t:
@ -572,11 +572,20 @@ public abstract class DecimalQuantity_AbstractBCD implements DecimalQuantity {
* Returns a long approximating the internal BCD. A long can only represent the integral part of the
* number.
*
* @param truncateIfOverflow if false and the number does NOT fit, fails with an assertion error.
* @return A 64-bit integer representation of the internal BCD.
*/
public long toLong() {
public long toLong(boolean truncateIfOverflow) {
// NOTE: Call sites should be guarded by fitsInLong(), like this:
// if (dq.fitsInLong()) { /* use dq.toLong() */ } else { /* use some fallback */ }
// Fallback behavior upon truncateIfOverflow is to truncate at 17 digits.
assert(truncateIfOverflow || fitsInLong());
long result = 0L;
for (int magnitude = scale + precision - 1; magnitude >= 0; magnitude--) {
int upperMagnitude = Math.min(scale + precision, lOptPos) - 1;
if (truncateIfOverflow) {
upperMagnitude = Math.min(upperMagnitude, 17);
}
for (int magnitude = upperMagnitude; magnitude >= 0; magnitude--) {
result = result * 10 + getDigitPos(magnitude - scale);
}
if (isNegative()) {
@ -593,10 +602,20 @@ public abstract class DecimalQuantity_AbstractBCD implements DecimalQuantity {
public long toFractionLong(boolean includeTrailingZeros) {
long result = 0L;
int magnitude = -1;
for (; (magnitude >= scale || (includeTrailingZeros && magnitude >= rReqPos))
&& magnitude >= rOptPos; magnitude--) {
int lowerMagnitude = Math.max(scale, rOptPos);
if (includeTrailingZeros) {
lowerMagnitude = Math.min(lowerMagnitude, rReqPos);
}
// NOTE: Java has only signed longs, so we check result <= 1e17 instead of 1e18
for (; magnitude >= lowerMagnitude && result <= 1e17; magnitude--) {
result = result * 10 + getDigitPos(magnitude - scale);
}
// Remove trailing zeros; this can happen during integer overflow cases.
if (!includeTrailingZeros) {
while (result > 0 && (result % 10) == 0) {
result /= 10;
}
}
return result;
}
@ -641,9 +660,9 @@ public abstract class DecimalQuantity_AbstractBCD implements DecimalQuantity {
*/
@Override
public double toDouble() {
if (isApproximate) {
return toDoubleFromOriginal();
}
// If this assertion fails, you need to call roundToInfinity() or some other rounding method.
// See the comment at the top of this file explaining the "isApproximate" field.
assert !isApproximate;
if (isNaN()) {
return Double.NaN;
@ -652,7 +671,7 @@ public abstract class DecimalQuantity_AbstractBCD implements DecimalQuantity {
}
// TODO: Do like in C++ and use a library function to perform this conversion?
// This code is not as not in Java because .parse() returns a BigDecimal, not a double.
// This code is not as hot in Java because .parse() returns a BigDecimal, not a double.
long tempLong = 0L;
int lostDigits = precision - Math.min(precision, 17);
@ -701,26 +720,6 @@ public abstract class DecimalQuantity_AbstractBCD implements DecimalQuantity {
return bcdToBigDecimal();
}
protected double toDoubleFromOriginal() {
double result = origDouble;
int delta = origDelta;
if (delta >= 0) {
// 1e22 is the largest exact double.
for (; delta >= 22; delta -= 22)
result *= 1e22;
result *= DOUBLE_MULTIPLIERS[delta];
} else {
// 1e22 is the largest exact double.
for (; delta <= -22; delta += 22)
result /= 1e22;
result /= DOUBLE_MULTIPLIERS[-delta];
}
if (isNegative()) {
result = -result;
}
return result;
}
private static int safeSubtract(int a, int b) {
int diff = a - b;
if (b < 0 && diff < a)
@ -952,7 +951,7 @@ public abstract class DecimalQuantity_AbstractBCD implements DecimalQuantity {
sb.append('0');
}
for (int m = getUpperDisplayMagnitude(); m >= getLowerDisplayMagnitude(); m--) {
sb.append(getDigit(m));
sb.append((char) ('0' + getDigit(m)));
if (m == 0)
sb.append('.');
}
@ -974,15 +973,20 @@ public abstract class DecimalQuantity_AbstractBCD implements DecimalQuantity {
result.append("0E+0");
return;
}
result.append((char) ('0' + getDigitPos(precision - 1)));
if (precision > 1) {
// NOTE: It is not safe to add to lOptPos (aka maxInt) or subtract from
// rOptPos (aka -maxFrac) due to overflow.
int upperPos = Math.min(precision + scale, lOptPos) - scale - 1;
int lowerPos = Math.max(scale, rOptPos) - scale;
int p = upperPos;
result.append((char) ('0' + getDigitPos(p)));
if ((--p) >= lowerPos) {
result.append('.');
for (int i = 1; i < precision; i++) {
result.append((char) ('0' + getDigitPos(precision - i - 1)));
for (; p >= lowerPos; p--) {
result.append((char) ('0' + getDigitPos(p)));
}
}
result.append('E');
int _scale = scale + precision - 1;
int _scale = upperPos + scale;
if (_scale < 0) {
_scale *= -1;
result.append('-');

2
icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/ParsedNumber.java
View file

@ -155,7 +155,7 @@ public class ParsedNumber {
}
if (quantity.fitsInLong() && !forceBigDecimal) {
return quantity.toLong();
return quantity.toLong(false);
} else {
return quantity.toBigDecimal();
}

16
icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/DecimalQuantityTest.java
View file

@ -496,6 +496,22 @@ public class DecimalQuantityTest extends TestFmwk {
}
}
@Test
public void testMaxDigits() {
DecimalQuantity_DualStorageBCD dq = new DecimalQuantity_DualStorageBCD(876.543);
dq.roundToInfinity();
dq.setIntegerLength(0, 2);
dq.setFractionLength(0, 2);
assertEquals("Should trim, toPlainString", "76.54", dq.toPlainString());
assertEquals("Should trim, toScientificString", "7.654E+1", dq.toScientificString());
assertEquals("Should trim, toLong", 76, dq.toLong(true));
assertEquals("Should trim, toFractionLong", 54, dq.toFractionLong(false));
if (!logKnownIssue("13701", "consider cleaning up")) {
assertEquals("Should trim, toDouble", 76.54, dq.toDouble());
assertEquals("Should trim, toBigDecimal", new BigDecimal("76.54"), dq.toBigDecimal());
}
}
static void assertDoubleEquals(String message, double d1, double d2) {
boolean equal = (Math.abs(d1 - d2) < 1e-6) || (Math.abs((d1 - d2) / d1) < 1e-6);
handleAssert(equal, message, d1, d2, null, false);

1
icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/ExhaustiveNumberTest.java
View file

@ -170,7 +170,6 @@ public class ExhaustiveNumberTest extends TestFmwk {
if (explicitRequired) {
assertTrue(alert + "Should be using approximate double", !fq.explicitExactDouble);
}
assertEquals(alert + "Initial construction from hard double", d, fq.toDouble());
fq.roundToInfinity();
if (explicitRequired) {
assertTrue(alert + "Should not be using approximate double", fq.explicitExactDouble);

3
icu4j/main/tests/framework/src/com/ibm/icu/dev/test/TestFmwk.java
View file

@ -169,6 +169,9 @@ abstract public class TestFmwk extends AbstractTestLog {
return false;
}
// TODO: This method currently does not do very much.
// See http://bugs.icu-project.org/trac/ticket/12589
StringBuffer descBuf = new StringBuffer();
// TODO(junit) : what to do about this?
//getParams().stack.appendPath(descBuf);