Skip to content

Commit

Permalink
ICU-21947 Replace FixedDecimal with DecimalQuantity in PluralRule sam…
Browse files Browse the repository at this point in the history 
…ple parsing

See #2007
  • Loading branch information
@echeran
echeran committed Aug 11, 2022
1 parent 0eecb25 commit 3ef03a4
Show file tree
Hide file tree
Showing 9 changed files with 755 additions and 641 deletions.
106 changes: 54 additions & 52 deletions icu4c/source/i18n/plurrule.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "hash.h"
#include "locutil.h"
#include "mutex.h"
#include "number_decnum.h"
#include "patternprops.h"
#include "plurrule_impl.h"
#include "putilimp.h"
Expand All @@ -45,7 +46,9 @@
U_NAMESPACE_BEGIN

using namespace icu::pluralimpl;
using icu::number::impl::DecNum;
using icu::number::impl::DecimalQuantity;
using icu::number::impl::RoundingMode;

static const UChar PLURAL_KEYWORD_OTHER[]={LOW_O,LOW_T,LOW_H,LOW_E,LOW_R,0};
static const UChar PLURAL_DEFAULT_RULE[]={LOW_O,LOW_T,LOW_H,LOW_E,LOW_R,COLON,SPACE,LOW_N,0};
Expand Down Expand Up @@ -369,36 +372,18 @@ PluralRules::getAllKeywordValues(const UnicodeString & /* keyword */, double * /
return 0;
}


static double scaleForInt(double d) {
double scale = 1.0;
while (d != floor(d)) {
d = d * 10.0;
scale = scale * 10.0;
}
return scale;
}

static const double powers10[7] = {1.0, 10.0, 100.0, 1000.0, 10000.0, 100000.0, 1000000.0}; // powers of 10 for 0..6
static double applyExponent(double source, int32_t exponent) {
if (exponent >= 0 && exponent <= 6) {
return source * powers10[exponent];
}
return source * pow(10.0, exponent);
}

/**
* Helper method for the overrides of getSamples() for double and FixedDecimal
* return value types. Provide only one of an allocated array of doubles or
* FixedDecimals, and a nullptr for the other.
* Helper method for the overrides of getSamples() for double and DecimalQuantity
* return value types. Provide only one of an allocated array of double or
* DecimalQuantity, and a nullptr for the other.
*/
static int32_t
getSamplesFromString(const UnicodeString &samples, double *destDbl,
FixedDecimal* destFd, int32_t destCapacity,
DecimalQuantity* destDq, int32_t destCapacity,
UErrorCode& status) {

if ((destDbl == nullptr && destFd == nullptr)
|| (destDbl != nullptr && destFd != nullptr)) {
if ((destDbl == nullptr && destDq == nullptr)
|| (destDbl != nullptr && destDq != nullptr)) {
status = U_INTERNAL_PROGRAM_ERROR;
return 0;
}
Expand All @@ -420,58 +405,75 @@ getSamplesFromString(const UnicodeString &samples, double *destDbl,
// std::cout << "PluralRules::getSamples(), samplesRange = \"" << sampleRange.toUTF8String(ss) << "\"\n";
int32_t tildeIndex = sampleRange.indexOf(TILDE);
if (tildeIndex < 0) {
FixedDecimal fixed(sampleRange, status);
DecimalQuantity dq = DecimalQuantity::fromExponentString(sampleRange, status);
if (isDouble) {
double sampleValue = fixed.source;
if (fixed.visibleDecimalDigitCount == 0 || sampleValue != floor(sampleValue)) {
destDbl[sampleCount++] = applyExponent(sampleValue, fixed.exponent);
// See warning note below about lack of precision for floating point samples for numbers with
// trailing zeroes in the decimal fraction representation.
double dblValue = dq.toDouble();
if (!(dblValue == floor(dblValue) && dq.fractionCount() > 0)) {
destDbl[sampleCount++] = dblValue;
}
} else {
destFd[sampleCount++] = fixed;
destDq[sampleCount++] = dq;
}
} else {
FixedDecimal fixedLo(sampleRange.tempSubStringBetween(0, tildeIndex), status);
FixedDecimal fixedHi(sampleRange.tempSubStringBetween(tildeIndex+1), status);
double rangeLo = fixedLo.source;
double rangeHi = fixedHi.source;
DecimalQuantity rangeLo =
DecimalQuantity::fromExponentString(sampleRange.tempSubStringBetween(0, tildeIndex), status);
DecimalQuantity rangeHi = DecimalQuantity::fromExponentString(sampleRange.tempSubStringBetween(tildeIndex+1), status);
if (U_FAILURE(status)) {
break;
}
if (rangeHi < rangeLo) {
if (rangeHi.toDouble() < rangeLo.toDouble()) {
status = U_INVALID_FORMAT_ERROR;
break;
}

// For ranges of samples with fraction decimal digits, scale the number up so that we
// are adding one in the units place. Avoids roundoffs from repetitive adds of tenths.
DecimalQuantity incrementDq;
incrementDq.setToInt(1);
int32_t lowerDispMag = rangeLo.getLowerDisplayMagnitude();
int32_t exponent = rangeLo.getExponent();
int32_t incrementScale = lowerDispMag + exponent;
incrementDq.adjustMagnitude(incrementScale);
double incrementVal = incrementDq.toDouble(); // 10 ^ incrementScale


double scale = scaleForInt(rangeLo);
double t = scaleForInt(rangeHi);
if (t > scale) {
scale = t;
}
rangeLo *= scale;
rangeHi *= scale;
for (double n=rangeLo; n<=rangeHi; n+=1) {
double sampleValue = n/scale;
DecimalQuantity dq(rangeLo);
double dblValue = dq.toDouble();
double end = rangeHi.toDouble();

while (dblValue <= end) {
if (isDouble) {
// Hack Alert: don't return any decimal samples with integer values that
// originated from a format with trailing decimals.
// This API is returning doubles, which can't distinguish having displayed
// zeros to the right of the decimal.
// This results in test failures with values mapping back to a different keyword.
if (!(sampleValue == floor(sampleValue) && fixedLo.visibleDecimalDigitCount > 0)) {
destDbl[sampleCount++] = sampleValue;
if (!(dblValue == floor(dblValue) && dq.fractionCount() > 0)) {
destDbl[sampleCount++] = dblValue;
}
} else {
int32_t v = (int32_t) fixedLo.getPluralOperand(PluralOperand::PLURAL_OPERAND_V);
int32_t e = (int32_t) fixedLo.getPluralOperand(PluralOperand::PLURAL_OPERAND_E);
FixedDecimal newSample = FixedDecimal::createWithExponent(sampleValue, v, e);
destFd[sampleCount++] = newSample;
destDq[sampleCount++] = dq;
}
if (sampleCount >= destCapacity) {
break;
}

// Increment dq for next iteration

// Because DecNum and DecimalQuantity do not support
// add operations, we need to convert to/from double,
// despite precision lossiness for decimal fractions like 0.1.
dblValue += incrementVal;
DecNum newDqDecNum;
newDqDecNum.setTo(dblValue, status);
DecimalQuantity newDq;
newDq.setToDecNum(newDqDecNum, status);
newDq.setMinFraction(-lowerDispMag);
newDq.roundToMagnitude(lowerDispMag, RoundingMode::UNUM_ROUND_HALFEVEN, status);
newDq.adjustMagnitude(-exponent);
newDq.adjustExponent(exponent);
dblValue = newDq.toDouble();
dq = newDq;
}
}
sampleStartIdx = sampleEndIdx + 1;
Expand Down Expand Up @@ -505,7 +507,7 @@ PluralRules::getSamples(const UnicodeString &keyword, double *dest,
}

int32_t
PluralRules::getSamples(const UnicodeString &keyword, FixedDecimal *dest,
PluralRules::getSamples(const UnicodeString &keyword, DecimalQuantity *dest,
int32_t destCapacity, UErrorCode& status) {
if (U_FAILURE(status)) {
return 0;
Expand Down
2 changes: 1 addition & 1 deletion icu4c/source/i18n/plurrule_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
* A FixedDecimal version of UPLRULES_NO_UNIQUE_VALUE used in PluralRulesTest
* for parsing of samples.
*/
#define UPLRULES_NO_UNIQUE_VALUE_DECIMAL (FixedDecimal((double)-0.00123456777))
#define UPLRULES_NO_UNIQUE_VALUE_DECIMAL(ERROR_CODE) (DecimalQuantity::fromExponentString(u"-0.00123456777", ERROR_CODE))

class PluralRulesTest;

Expand Down
10 changes: 8 additions & 2 deletions icu4c/source/i18n/unicode/plurrule.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,15 @@ class FormattedNumber;
class FormattedNumberRange;
namespace impl {
class UFormattedNumberRangeData;
class DecimalQuantity;
class DecNum;
}
}

#ifndef U_HIDE_INTERNAL_API
using icu::number::impl::DecimalQuantity;
#endif /* U_HIDE_INTERNAL_API */

/**
* Defines rules for mapping non-negative numeric values onto a small set of
* keywords. Rules are constructed from a text description, consisting
Expand Down Expand Up @@ -468,7 +474,7 @@ class U_I18N_API PluralRules : public UObject {

#ifndef U_HIDE_INTERNAL_API
/**
* Internal-only function that returns FixedDecimals instead of doubles.
* Internal-only function that returns DecimalQuantitys instead of doubles.
*
* Returns sample values for which select() would return the keyword. If
* the keyword is unknown, returns no values, but this is not an error.
Expand All @@ -488,7 +494,7 @@ class U_I18N_API PluralRules : public UObject {
* @internal
*/
int32_t getSamples(const UnicodeString &keyword,
FixedDecimal *dest, int32_t destCapacity,
DecimalQuantity *dest, int32_t destCapacity,
UErrorCode& status);
#endif /* U_HIDE_INTERNAL_API */

Expand Down
Loading

1 comment on commit 3ef03a4

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: 3ef03a4 Previous: 0eecb25 Ratio
TestCharsetEncoderICU 6.02019854270491 ns/iter 2.2648393881898277 ns/iter 2.66

This comment was automatically generated by workflow using github-action-benchmark.

Please sign in to comment.