From f7ab1f7c5007e53993fc3b907ababf5b04ce22bc Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Tue, 17 Nov 2020 10:06:57 +0000 Subject: [PATCH] ICU-21349 Fix confusing names of exponent variables. See #1464 --- icu4c/source/i18n/units_converter.cpp | 43 +++---- icu4c/source/i18n/units_converter.h | 22 ++-- .../com/ibm/icu/impl/units/UnitConverter.java | 116 ++++++++++-------- 3 files changed, 97 insertions(+), 84 deletions(-) diff --git a/icu4c/source/i18n/units_converter.cpp b/icu4c/source/i18n/units_converter.cpp index 9c163537032..bed40dd4e13 100644 --- a/icu4c/source/i18n/units_converter.cpp +++ b/icu4c/source/i18n/units_converter.cpp @@ -26,7 +26,7 @@ void U_I18N_API Factor::multiplyBy(const Factor &rhs) { factorNum *= rhs.factorNum; factorDen *= rhs.factorDen; for (int i = 0; i < CONSTANTS_COUNT; i++) { - constants[i] += rhs.constants[i]; + constantExponents[i] += rhs.constantExponents[i]; } // NOTE @@ -39,7 +39,7 @@ void U_I18N_API Factor::divideBy(const Factor &rhs) { factorNum *= rhs.factorDen; factorDen *= rhs.factorNum; for (int i = 0; i < CONSTANTS_COUNT; i++) { - constants[i] -= rhs.constants[i]; + constantExponents[i] -= rhs.constantExponents[i]; } // NOTE @@ -51,7 +51,7 @@ void U_I18N_API Factor::divideBy(const Factor &rhs) { void U_I18N_API Factor::power(int32_t power) { // multiply all the constant by the power. for (int i = 0; i < CONSTANTS_COUNT; i++) { - constants[i] *= power; + constantExponents[i] *= power; } bool shouldFlip = power < 0; // This means that after applying the absolute power, we should flip @@ -66,14 +66,6 @@ void U_I18N_API Factor::power(int32_t power) { } } -void U_I18N_API Factor::flip() { - std::swap(factorNum, factorDen); - - for (int i = 0; i < CONSTANTS_COUNT; i++) { - constants[i] *= -1; - } -} - void U_I18N_API Factor::applySiPrefix(UMeasureSIPrefix siPrefix) { if (siPrefix == UMeasureSIPrefix::UMEASURE_SI_PREFIX_ONE) return; // No need to do anything @@ -89,12 +81,12 @@ void U_I18N_API Factor::applySiPrefix(UMeasureSIPrefix siPrefix) { void U_I18N_API Factor::substituteConstants() { for (int i = 0; i < CONSTANTS_COUNT; i++) { - if (this->constants[i] == 0) { + if (this->constantExponents[i] == 0) { continue; } - auto absPower = std::abs(this->constants[i]); - Signum powerSig = this->constants[i] < 0 ? Signum::NEGATIVE : Signum::POSITIVE; + auto absPower = std::abs(this->constantExponents[i]); + Signum powerSig = this->constantExponents[i] < 0 ? Signum::NEGATIVE : Signum::POSITIVE; double absConstantValue = std::pow(constantsValues[i], absPower); if (powerSig == Signum::NEGATIVE) { @@ -103,7 +95,7 @@ void U_I18N_API Factor::substituteConstants() { this->factorNum *= absConstantValue; } - this->constants[i] = 0; + this->constantExponents[i] = 0; } } @@ -273,6 +265,7 @@ UBool checkSimpleUnit(const MeasureUnitImpl &unit, UErrorCode &status) { /** * Extract conversion rate from `source` to `target` */ +// In ICU4J, this function is partially inlined in the UnitConverter constructor. void loadConversionRate(ConversionRate &conversionRate, const MeasureUnitImpl &source, const MeasureUnitImpl &target, Convertibility unitsState, const ConversionRates &ratesInfo, UErrorCode &status) { @@ -361,28 +354,28 @@ UBool checkAllDimensionsAreZeros(const MaybeStackVector & void U_I18N_API addSingleFactorConstant(StringPiece baseStr, int32_t power, Signum signum, Factor &factor, UErrorCode &status) { if (baseStr == "ft_to_m") { - factor.constants[CONSTANT_FT2M] += power * signum; + factor.constantExponents[CONSTANT_FT2M] += power * signum; } else if (baseStr == "ft2_to_m2") { - factor.constants[CONSTANT_FT2M] += 2 * power * signum; + factor.constantExponents[CONSTANT_FT2M] += 2 * power * signum; } else if (baseStr == "ft3_to_m3") { - factor.constants[CONSTANT_FT2M] += 3 * power * signum; + factor.constantExponents[CONSTANT_FT2M] += 3 * power * signum; } else if (baseStr == "in3_to_m3") { - factor.constants[CONSTANT_FT2M] += 3 * power * signum; + factor.constantExponents[CONSTANT_FT2M] += 3 * power * signum; factor.factorDen *= 12 * 12 * 12; } else if (baseStr == "gal_to_m3") { factor.factorNum *= 231; - factor.constants[CONSTANT_FT2M] += 3 * power * signum; + factor.constantExponents[CONSTANT_FT2M] += 3 * power * signum; factor.factorDen *= 12 * 12 * 12; } else if (baseStr == "gal_imp_to_m3") { - factor.constants[CONSTANT_GAL_IMP2M3] += power * signum; + factor.constantExponents[CONSTANT_GAL_IMP2M3] += power * signum; } else if (baseStr == "G") { - factor.constants[CONSTANT_G] += power * signum; + factor.constantExponents[CONSTANT_G] += power * signum; } else if (baseStr == "gravity") { - factor.constants[CONSTANT_GRAVITY] += power * signum; + factor.constantExponents[CONSTANT_GRAVITY] += power * signum; } else if (baseStr == "lb_to_kg") { - factor.constants[CONSTANT_LB2KG] += power * signum; + factor.constantExponents[CONSTANT_LB2KG] += power * signum; } else if (baseStr == "PI") { - factor.constants[CONSTANT_PI] += power * signum; + factor.constantExponents[CONSTANT_PI] += power * signum; } else { if (signum == Signum::NEGATIVE) { factor.factorDen *= std::pow(strToDouble(baseStr, status), power); diff --git a/icu4c/source/i18n/units_converter.h b/icu4c/source/i18n/units_converter.h index b2c1ce8fdc4..f71628da35e 100644 --- a/icu4c/source/i18n/units_converter.h +++ b/icu4c/source/i18n/units_converter.h @@ -20,11 +20,12 @@ namespace units { /* Internal Structure */ +// Constants corresponding to unitConstants in CLDR's units.xml. enum Constants { - CONSTANT_FT2M, // ft2m stands for foot to meter. - CONSTANT_PI, // PI - CONSTANT_GRAVITY, // Gravity - CONSTANT_G, + CONSTANT_FT2M, // ft_to_m + CONSTANT_PI, // PI + CONSTANT_GRAVITY, // Gravity of earth (9.80665 m/s^2), "g". + CONSTANT_G, // Newtonian constant of gravitation, "G". CONSTANT_GAL_IMP2M3, // Gallon imp to m3 CONSTANT_LB2KG, // Pound to Kilogram @@ -36,6 +37,7 @@ enum Constants { // resources file. A unit test checks that all constants in the resource // file are at least recognised by the code. Derived constants' values or // hard-coded derivations are not checked. +// In ICU4J, these constants live in UnitConverter.Factor.getConversionRate(). static const double constantsValues[CONSTANTS_COUNT] = { 0.3048, // CONSTANT_FT2M 411557987.0 / 131002976.0, // CONSTANT_PI @@ -56,7 +58,9 @@ struct U_I18N_API Factor { double factorDen = 1; double offset = 0; bool reciprocal = false; - int32_t constants[CONSTANTS_COUNT] = {}; + + // Exponents for the symbolic constants + int32_t constantExponents[CONSTANTS_COUNT] = {}; void multiplyBy(const Factor &rhs); void divideBy(const Factor &rhs); @@ -64,11 +68,13 @@ struct U_I18N_API Factor { // Apply the power to the factor. void power(int32_t power); - // Flip the `Factor`, for example, factor= 2/3, flippedFactor = 3/2 - void flip(); - // Apply SI prefix to the `Factor` void applySiPrefix(UMeasureSIPrefix siPrefix); + + // Does an in-place substition of the "symbolic constants" based on + // constantExponents (resetting the exponents). + // + // In ICU4J, see UnitConverter.Factor.getConversionRate(). void substituteConstants(); }; diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitConverter.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitConverter.java index 016282ef455..90e29b9c52e 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitConverter.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitConverter.java @@ -96,8 +96,6 @@ public class UnitConverter { UNCONVERTIBLE, } - // TODO: improve documentation and Constant implementation - /** * Responsible for all the Factor operation * NOTE: @@ -106,15 +104,21 @@ public class UnitConverter { static class Factor { private BigDecimal factorNum; private BigDecimal factorDen; - /* FACTOR CONSTANTS */ - private int - CONSTANT_FT2M = 0, // ft2m stands for foot to meter. - CONSTANT_PI = 0, // PI - CONSTANT_GRAVITY = 0, // Gravity - CONSTANT_G = 0, - CONSTANT_GAL_IMP2M3 = 0, // Gallon imp to m3 - CONSTANT_LB2KG = 0; // Pound to Kilogram + // The exponents below correspond to ICU4C's Factor::exponents[]. + + /** Exponent for the ft_to_m constant */ + private int exponentFtToM = 0; + /** Exponent for PI */ + private int exponentPi = 0; + /** Exponent for gravity (gravity-of-earth, "g") */ + private int exponentGravity = 0; + /** Exponent for Newtonian constant of gravitation "G". */ + private int exponentG = 0; + /** Exponent for the imperial-gallon to cubic-meter conversion rate constant */ + private int exponentGalImpToM3 = 0; + /** Exponent for the pound to kilogram conversion rate constant */ + private int exponentLbToKg = 0; /** * Creates Empty Factor @@ -160,33 +164,43 @@ public class UnitConverter { result.factorNum = this.factorNum; result.factorDen = this.factorDen; - result.CONSTANT_FT2M = this.CONSTANT_FT2M; - result.CONSTANT_PI = this.CONSTANT_PI; - result.CONSTANT_GRAVITY = this.CONSTANT_GRAVITY; - result.CONSTANT_G = this.CONSTANT_G; - result.CONSTANT_GAL_IMP2M3 = this.CONSTANT_GAL_IMP2M3; - result.CONSTANT_LB2KG = this.CONSTANT_LB2KG; + result.exponentFtToM = this.exponentFtToM; + result.exponentPi = this.exponentPi; + result.exponentGravity = this.exponentGravity; + result.exponentG = this.exponentG; + result.exponentGalImpToM3 = this.exponentGalImpToM3; + result.exponentLbToKg = this.exponentLbToKg; return result; } /** * Returns a single `BigDecimal` that represent the conversion rate after substituting all the constants. + * + * In ICU4C, see Factor::substituteConstants(). */ public BigDecimal getConversionRate() { + // TODO: this copies all the exponents then doesn't use them at all. Factor resultCollector = this.copy(); - resultCollector.substitute(new BigDecimal("0.3048"), this.CONSTANT_FT2M); - resultCollector.substitute(new BigDecimal("411557987.0").divide(new BigDecimal("131002976.0"), DECIMAL128), this.CONSTANT_PI); - resultCollector.substitute(new BigDecimal("9.80665"), this.CONSTANT_GRAVITY); - resultCollector.substitute(new BigDecimal("6.67408E-11"), this.CONSTANT_G); - resultCollector.substitute(new BigDecimal("0.00454609"), this.CONSTANT_GAL_IMP2M3); - resultCollector.substitute(new BigDecimal("0.45359237"), this.CONSTANT_LB2KG); + // TODO(icu-units#92): port C++ unit tests to Java. + // These values are a hard-coded subset of unitConstants in the + // units resources file. A unit test should check that all constants + // in the resource file are at least recognised by the code. + // In ICU4C, these constants live in constantsValues[]. + resultCollector.multiply(new BigDecimal("0.3048"), this.exponentFtToM); + // TODO: this recalculates this division every time this is called. + resultCollector.multiply(new BigDecimal("411557987.0").divide(new BigDecimal("131002976.0"), DECIMAL128), this.exponentPi); + resultCollector.multiply(new BigDecimal("9.80665"), this.exponentGravity); + resultCollector.multiply(new BigDecimal("6.67408E-11"), this.exponentG); + resultCollector.multiply(new BigDecimal("0.00454609"), this.exponentGalImpToM3); + resultCollector.multiply(new BigDecimal("0.45359237"), this.exponentLbToKg); return resultCollector.factorNum.divide(resultCollector.factorDen, DECIMAL128); } - private void substitute(BigDecimal value, int power) { + /** Multiplies the Factor instance by value^power. */ + private void multiply(BigDecimal value, int power) { if (power == 0) return; BigDecimal absPoweredValue = value.pow(Math.abs(power), DECIMAL128); @@ -225,12 +239,12 @@ public class UnitConverter { result.factorDen = this.factorNum.pow(power * -1); } - result.CONSTANT_FT2M = this.CONSTANT_FT2M * power; - result.CONSTANT_PI = this.CONSTANT_PI * power; - result.CONSTANT_GRAVITY = this.CONSTANT_GRAVITY * power; - result.CONSTANT_G = this.CONSTANT_G * power; - result.CONSTANT_GAL_IMP2M3 = this.CONSTANT_GAL_IMP2M3 * power; - result.CONSTANT_LB2KG = this.CONSTANT_LB2KG * power; + result.exponentFtToM = this.exponentFtToM * power; + result.exponentPi = this.exponentPi * power; + result.exponentGravity = this.exponentGravity * power; + result.exponentG = this.exponentG * power; + result.exponentGalImpToM3 = this.exponentGalImpToM3 * power; + result.exponentLbToKg = this.exponentLbToKg * power; return result; } @@ -240,12 +254,12 @@ public class UnitConverter { result.factorNum = this.factorNum.multiply(other.factorDen); result.factorDen = this.factorDen.multiply(other.factorNum); - result.CONSTANT_FT2M = this.CONSTANT_FT2M - other.CONSTANT_FT2M; - result.CONSTANT_PI = this.CONSTANT_PI - other.CONSTANT_PI; - result.CONSTANT_GRAVITY = this.CONSTANT_GRAVITY - other.CONSTANT_GRAVITY; - result.CONSTANT_G = this.CONSTANT_G - other.CONSTANT_G; - result.CONSTANT_GAL_IMP2M3 = this.CONSTANT_GAL_IMP2M3 - other.CONSTANT_GAL_IMP2M3; - result.CONSTANT_LB2KG = this.CONSTANT_LB2KG - other.CONSTANT_LB2KG; + result.exponentFtToM = this.exponentFtToM - other.exponentFtToM; + result.exponentPi = this.exponentPi - other.exponentPi; + result.exponentGravity = this.exponentGravity - other.exponentGravity; + result.exponentG = this.exponentG - other.exponentG; + result.exponentGalImpToM3 = this.exponentGalImpToM3 - other.exponentGalImpToM3; + result.exponentLbToKg = this.exponentLbToKg - other.exponentLbToKg; return result; } @@ -255,12 +269,12 @@ public class UnitConverter { result.factorNum = this.factorNum.multiply(other.factorNum); result.factorDen = this.factorDen.multiply(other.factorDen); - result.CONSTANT_FT2M = this.CONSTANT_FT2M + other.CONSTANT_FT2M; - result.CONSTANT_PI = this.CONSTANT_PI + other.CONSTANT_PI; - result.CONSTANT_GRAVITY = this.CONSTANT_GRAVITY + other.CONSTANT_GRAVITY; - result.CONSTANT_G = this.CONSTANT_G + other.CONSTANT_G; - result.CONSTANT_GAL_IMP2M3 = this.CONSTANT_GAL_IMP2M3 + other.CONSTANT_GAL_IMP2M3; - result.CONSTANT_LB2KG = this.CONSTANT_LB2KG + other.CONSTANT_LB2KG; + result.exponentFtToM = this.exponentFtToM + other.exponentFtToM; + result.exponentPi = this.exponentPi + other.exponentPi; + result.exponentGravity = this.exponentGravity + other.exponentGravity; + result.exponentG = this.exponentG + other.exponentG; + result.exponentGalImpToM3 = this.exponentGalImpToM3 + other.exponentGalImpToM3; + result.exponentLbToKg = this.exponentLbToKg + other.exponentLbToKg; return result; } @@ -280,28 +294,28 @@ public class UnitConverter { private void addEntity(String entity, int power) { if ("ft_to_m".equals(entity)) { - this.CONSTANT_FT2M += power; + this.exponentFtToM += power; } else if ("ft2_to_m2".equals(entity)) { - this.CONSTANT_FT2M += 2 * power; + this.exponentFtToM += 2 * power; } else if ("ft3_to_m3".equals(entity)) { - this.CONSTANT_FT2M += 3 * power; + this.exponentFtToM += 3 * power; } else if ("in3_to_m3".equals(entity)) { - this.CONSTANT_FT2M += 3 * power; + this.exponentFtToM += 3 * power; this.factorDen = this.factorDen.multiply(BigDecimal.valueOf(Math.pow(12, 3))); } else if ("gal_to_m3".equals(entity)) { this.factorNum = this.factorNum.multiply(BigDecimal.valueOf(231)); - this.CONSTANT_FT2M += 3 * power; + this.exponentFtToM += 3 * power; this.factorDen = this.factorDen.multiply(BigDecimal.valueOf(12 * 12 * 12)); } else if ("gal_imp_to_m3".equals(entity)) { - this.CONSTANT_GAL_IMP2M3 += power; + this.exponentGalImpToM3 += power; } else if ("G".equals(entity)) { - this.CONSTANT_G += power; + this.exponentG += power; } else if ("gravity".equals(entity)) { - this.CONSTANT_GRAVITY += power; + this.exponentGravity += power; } else if ("lb_to_kg".equals(entity)) { - this.CONSTANT_LB2KG += power; + this.exponentLbToKg += power; } else if ("PI".equals(entity)) { - this.CONSTANT_PI += power; + this.exponentPi += power; } else { BigDecimal decimalEntity = new BigDecimal(entity).pow(power, DECIMAL128); this.factorNum = this.factorNum.multiply(decimalEntity);