From 678b3d5f4a0d364d514e50b6e47764bd4c64901d Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Tue, 26 May 2020 19:39:01 +0200 Subject: [PATCH 1/6] Unit test focusing on hard-coded constants. --- icu4c/source/test/intltest/unitstest.cpp | 75 ++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/icu4c/source/test/intltest/unitstest.cpp b/icu4c/source/test/intltest/unitstest.cpp index b67f9adccd5..9456136c621 100644 --- a/icu4c/source/test/intltest/unitstest.cpp +++ b/icu4c/source/test/intltest/unitstest.cpp @@ -18,10 +18,12 @@ #include "unicode/measunit.h" #include "unicode/unistr.h" #include "unicode/unum.h" +#include "unicode/ures.h" #include "unitconverter.h" #include "unitsdata.h" #include "unitsrouter.h" #include "uparse.h" +#include "uresimp.h" struct UnitConversionTestCase { const StringPiece source; @@ -39,6 +41,7 @@ class UnitsTest : public IntlTest { void runIndexedTest(int32_t index, UBool exec, const char *&name, char *par = NULL); + void testHardcodeFreshnessForConvertUnits(); void testConversionCapability(); void testConversions(); void testPreferences(); @@ -55,6 +58,7 @@ void UnitsTest::runIndexedTest(int32_t index, UBool exec, const char *&name, cha logln("TestSuite UnitsTest: "); } TESTCASE_AUTO_BEGIN; + TESTCASE_AUTO(testHardcodeFreshnessForConvertUnits); TESTCASE_AUTO(testConversionCapability); TESTCASE_AUTO(testConversions); TESTCASE_AUTO(testPreferences); @@ -65,6 +69,77 @@ void UnitsTest::runIndexedTest(int32_t index, UBool exec, const char *&name, cha TESTCASE_AUTO_END; } +// Tests the hard-coded constants in the code against constants and unit +// identifiers that appear in units.txt, by simply parsing unit identifiers and +// then loading each conversion rate from the convertUnits resource into +// UnitConverter. +// +// MeasureUnit::forIdentifier() will fail for invalid unit identifiers. +// +// UnitConverter() returns an error if it fails to recognise a constant, which +// would thereby alert us if the hard-coded constants are stale. +void UnitsTest::testHardcodeFreshnessForConvertUnits() { + IcuTestErrorCode status(*this, "testConstantFreshness"); + LocalUResourceBundlePointer unitsBundle(ures_openDirect(NULL, "units", status)); + LocalUResourceBundlePointer unitConstants( + ures_getByKey(unitsBundle.getAlias(), "unitConstants", NULL, status)); + LocalUResourceBundlePointer convertUnits( + ures_getByKey(unitsBundle.getAlias(), "convertUnits", NULL, status)); + + CharString constants; + while (ures_hasNext(unitConstants.getAlias())) { + int32_t len; + const char *constant = NULL; + ures_getNextString(unitConstants.getAlias(), &len, &constant, status); + constants.append(" \"", status); + constants.append(constant, status); + constants.append("\"", status); + } + + if (status.errDataIfFailureAndReset("ures_getByKey(unitsBundle, \"convertUnits\", ...)")) { + return; + } + ConversionRates conversionRates(status); + StackUResourceBundle stackBundle; + while (ures_hasNext(convertUnits.getAlias())) { + ures_getNextResource(convertUnits.getAlias(), stackBundle.getAlias(), status); + const char *sourceKey = ures_getKey(stackBundle.getAlias()); + + int32_t targetLen; + const UChar *uTarget = ures_getStringByKey(stackBundle.getAlias(), "target", &targetLen, status); + CharString target; + target.appendInvariantChars(uTarget, targetLen, status); + + int32_t factorLen; + const UChar *uFactor = ures_getStringByKey(stackBundle.getAlias(), "factor", &factorLen, status); + CharString factor; + factor.appendInvariantChars(uFactor, factorLen, status); + + if (status.errDataIfFailureAndReset("Resource loading failure")) { return; } + MeasureUnit sourceUnit = MeasureUnit::forIdentifier(sourceKey, status); + MeasureUnit targetUnit = MeasureUnit::forIdentifier(target.data(), status); + if (status.errDataIfFailureAndReset( + "MeasureUnit::forIdentifier(\"%s\", status); " + "MeasureUnit::forIdentifier(\"%s\", status);\n\n" + "If U_ILLEGAL_ARGUMENT_ERROR, please check that " + "\"icu4c/source/i18n/measunit_extra.cpp\" has the necessary unit identifiers in the " + "gSimpleUnits[] array. (Also check the gSubTypes[] array in measunit.cpp?)", + sourceKey, target.data())) { + continue; + } + UnitConverter(sourceUnit, targetUnit, conversionRates, status); + if (status.errDataIfFailureAndReset( + "UnitConverter(<%s>, <%s>, status).\n\n" + "If U_INVALID_FORMAT_ERROR, please check that \"icu4c/source/i18n/unitconverter.cpp\" " + "has all constants?\n" + "Factor for \"%s\" is: \"%s\".\n" + "Full list of constants:%s", + sourceKey, target.data(), sourceKey, factor.data(), constants.data())) { + continue; + } + } +} + void UnitsTest::testConversionCapability() { struct TestCase { const char *const source; From 832c19b0c66f6fda888da0a842bf8b5a9df64089 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Wed, 10 Jun 2020 14:47:42 +0200 Subject: [PATCH 2/6] Test that addSingleFactorConstant knowns all unitConstants. --- icu4c/source/data/misc/units.txt | 1 + icu4c/source/i18n/unitconverter.cpp | 5 +- icu4c/source/test/intltest/unitstest.cpp | 73 ++++++------------------ 3 files changed, 21 insertions(+), 58 deletions(-) diff --git a/icu4c/source/data/misc/units.txt b/icu4c/source/data/misc/units.txt index bd19b241cf7..723463b8aa0 100644 --- a/icu4c/source/data/misc/units.txt +++ b/icu4c/source/data/misc/units.txt @@ -425,6 +425,7 @@ units:table(nofallback){ gravity{"9.80665"} in3_to_m3{"ft3_to_m3/12*12*12"} lb_to_kg{"0.45359237"} + foobar{"1.234"} } unitPreferenceData{ "area"{ diff --git a/icu4c/source/i18n/unitconverter.cpp b/icu4c/source/i18n/unitconverter.cpp index a0f2efce385..1d7f0fa73d6 100644 --- a/icu4c/source/i18n/unitconverter.cpp +++ b/icu4c/source/i18n/unitconverter.cpp @@ -318,9 +318,12 @@ void loadConversionRate(ConversionRate &conversionRate, const MeasureUnit &sourc } // namespace +// Conceptually, this modifies factor: factor *= baseStr^(signum*power). +// +// baseStr must be a known constant or a value that strToDouble() is able to +// parse. 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; } else if (baseStr == "ft2_to_m2") { diff --git a/icu4c/source/test/intltest/unitstest.cpp b/icu4c/source/test/intltest/unitstest.cpp index 9456136c621..013dd8ef913 100644 --- a/icu4c/source/test/intltest/unitstest.cpp +++ b/icu4c/source/test/intltest/unitstest.cpp @@ -41,7 +41,7 @@ class UnitsTest : public IntlTest { void runIndexedTest(int32_t index, UBool exec, const char *&name, char *par = NULL); - void testHardcodeFreshnessForConvertUnits(); + void testUnitConstantFreshness(); void testConversionCapability(); void testConversions(); void testPreferences(); @@ -58,7 +58,7 @@ void UnitsTest::runIndexedTest(int32_t index, UBool exec, const char *&name, cha logln("TestSuite UnitsTest: "); } TESTCASE_AUTO_BEGIN; - TESTCASE_AUTO(testHardcodeFreshnessForConvertUnits); + TESTCASE_AUTO(testUnitConstantFreshness); TESTCASE_AUTO(testConversionCapability); TESTCASE_AUTO(testConversions); TESTCASE_AUTO(testPreferences); @@ -69,74 +69,33 @@ void UnitsTest::runIndexedTest(int32_t index, UBool exec, const char *&name, cha TESTCASE_AUTO_END; } -// Tests the hard-coded constants in the code against constants and unit -// identifiers that appear in units.txt, by simply parsing unit identifiers and -// then loading each conversion rate from the convertUnits resource into -// UnitConverter. -// -// MeasureUnit::forIdentifier() will fail for invalid unit identifiers. -// -// UnitConverter() returns an error if it fails to recognise a constant, which -// would thereby alert us if the hard-coded constants are stale. -void UnitsTest::testHardcodeFreshnessForConvertUnits() { - IcuTestErrorCode status(*this, "testConstantFreshness"); +// Tests the hard-coded constants in the code against constants that appear in +// units.txt. +void UnitsTest::testUnitConstantFreshness() { + IcuTestErrorCode status(*this, "testUnitConstantFreshness"); LocalUResourceBundlePointer unitsBundle(ures_openDirect(NULL, "units", status)); LocalUResourceBundlePointer unitConstants( ures_getByKey(unitsBundle.getAlias(), "unitConstants", NULL, status)); - LocalUResourceBundlePointer convertUnits( - ures_getByKey(unitsBundle.getAlias(), "convertUnits", NULL, status)); - CharString constants; while (ures_hasNext(unitConstants.getAlias())) { int32_t len; const char *constant = NULL; ures_getNextString(unitConstants.getAlias(), &len, &constant, status); - constants.append(" \"", status); - constants.append(constant, status); - constants.append("\"", status); - } - if (status.errDataIfFailureAndReset("ures_getByKey(unitsBundle, \"convertUnits\", ...)")) { - return; - } - ConversionRates conversionRates(status); - StackUResourceBundle stackBundle; - while (ures_hasNext(convertUnits.getAlias())) { - ures_getNextResource(convertUnits.getAlias(), stackBundle.getAlias(), status); - const char *sourceKey = ures_getKey(stackBundle.getAlias()); - - int32_t targetLen; - const UChar *uTarget = ures_getStringByKey(stackBundle.getAlias(), "target", &targetLen, status); - CharString target; - target.appendInvariantChars(uTarget, targetLen, status); - - int32_t factorLen; - const UChar *uFactor = ures_getStringByKey(stackBundle.getAlias(), "factor", &factorLen, status); - CharString factor; - factor.appendInvariantChars(uFactor, factorLen, status); - - if (status.errDataIfFailureAndReset("Resource loading failure")) { return; } - MeasureUnit sourceUnit = MeasureUnit::forIdentifier(sourceKey, status); - MeasureUnit targetUnit = MeasureUnit::forIdentifier(target.data(), status); + Factor factor; + addSingleFactorConstant(constant, 2, POSITIVE, factor, status); if (status.errDataIfFailureAndReset( - "MeasureUnit::forIdentifier(\"%s\", status); " - "MeasureUnit::forIdentifier(\"%s\", status);\n\n" - "If U_ILLEGAL_ARGUMENT_ERROR, please check that " - "\"icu4c/source/i18n/measunit_extra.cpp\" has the necessary unit identifiers in the " - "gSimpleUnits[] array. (Also check the gSubTypes[] array in measunit.cpp?)", - sourceKey, target.data())) { - continue; - } - UnitConverter(sourceUnit, targetUnit, conversionRates, status); - if (status.errDataIfFailureAndReset( - "UnitConverter(<%s>, <%s>, status).\n\n" + "addSingleFactorConstant(<%s>, ...).\n\n" "If U_INVALID_FORMAT_ERROR, please check that \"icu4c/source/i18n/unitconverter.cpp\" " - "has all constants?\n" - "Factor for \"%s\" is: \"%s\".\n" - "Full list of constants:%s", - sourceKey, target.data(), sourceKey, factor.data(), constants.data())) { + "has all constants? Is \"%s\" a new constant?\n", + constant, constant)) { continue; } + // TODO(units,hugovdm): implement some symbolic maths to evaluate the + // values of these constants? Counter-argument: constant values don't + // change, and the data-driven unit tests generally take care of + // validating the precision of conversions, if they have enough + // coverage. } } From 8948cc5ab195def83d258d7b39537b7a9e960f9e Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Fri, 10 Jul 2020 21:27:00 +0200 Subject: [PATCH 3/6] Remove the 'foobar' fake new constant (was used to test the tests). --- icu4c/source/data/misc/units.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/icu4c/source/data/misc/units.txt b/icu4c/source/data/misc/units.txt index 723463b8aa0..bd19b241cf7 100644 --- a/icu4c/source/data/misc/units.txt +++ b/icu4c/source/data/misc/units.txt @@ -425,7 +425,6 @@ units:table(nofallback){ gravity{"9.80665"} in3_to_m3{"ft3_to_m3/12*12*12"} lb_to_kg{"0.45359237"} - foobar{"1.234"} } unitPreferenceData{ "area"{ From dca04813795671ceab8832703ee3cbd59ab33a79 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Wed, 15 Jul 2020 15:19:16 +0200 Subject: [PATCH 4/6] Parse simple constants and check their hard-coded values. --- icu4c/source/test/intltest/unitstest.cpp | 30 +++++++++++++++++++----- 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/icu4c/source/test/intltest/unitstest.cpp b/icu4c/source/test/intltest/unitstest.cpp index 013dd8ef913..ba208ae1a63 100644 --- a/icu4c/source/test/intltest/unitstest.cpp +++ b/icu4c/source/test/intltest/unitstest.cpp @@ -83,7 +83,7 @@ void UnitsTest::testUnitConstantFreshness() { ures_getNextString(unitConstants.getAlias(), &len, &constant, status); Factor factor; - addSingleFactorConstant(constant, 2, POSITIVE, factor, status); + addSingleFactorConstant(constant, 1, POSITIVE, factor, status); if (status.errDataIfFailureAndReset( "addSingleFactorConstant(<%s>, ...).\n\n" "If U_INVALID_FORMAT_ERROR, please check that \"icu4c/source/i18n/unitconverter.cpp\" " @@ -91,11 +91,29 @@ void UnitsTest::testUnitConstantFreshness() { constant, constant)) { continue; } - // TODO(units,hugovdm): implement some symbolic maths to evaluate the - // values of these constants? Counter-argument: constant values don't - // change, and the data-driven unit tests generally take care of - // validating the precision of conversions, if they have enough - // coverage. + + // Check the values of constants that have a simple numeric value + factor.substituteConstants(); + int32_t uLen; + UnicodeString uVal = ures_getStringByKey(unitConstants.getAlias(), constant, &uLen, status); + CharString val; + val.appendInvariantChars(uVal, status); + if (status.errDataIfFailureAndReset("Failed to get constant value for %s.", constant)) { + continue; + } + DecimalQuantity dqVal; + UErrorCode parseStatus = U_ZERO_ERROR; + // TODO(units): unify with strToDouble() in unitconverter.cpp + dqVal.setToDecNumber(val.toStringPiece(), parseStatus); + if (!U_SUCCESS(parseStatus)) { + // Not simple to parse, skip validating this constant's value. (We + // leave catching mistakes to the data-driven integration tests.) + continue; + } + double expectedNumerator = dqVal.toDouble(); + assertEquals(UnicodeString("Constant ") + constant + u" numerator", expectedNumerator, + factor.factorNum); + assertEquals(UnicodeString("Constant ") + constant + u" denominator", 1.0, factor.factorDen); } } From e9fc257545270dd416ad9d2c61a5bb2258889edb Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Wed, 15 Jul 2020 16:17:34 +0200 Subject: [PATCH 5/6] Declare constantsValues as static const, with C99 initializer. --- icu4c/source/i18n/unitconverter.cpp | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/icu4c/source/i18n/unitconverter.cpp b/icu4c/source/i18n/unitconverter.cpp index 1d7f0fa73d6..2bb5ee3f141 100644 --- a/icu4c/source/i18n/unitconverter.cpp +++ b/icu4c/source/i18n/unitconverter.cpp @@ -84,15 +84,19 @@ void Factor::applySiPrefix(UMeasureSIPrefix siPrefix) { } void Factor::substituteConstants() { - double constantsValues[CONSTANTS_COUNT]; - - // TODO: Load those constant values from units data. - constantsValues[CONSTANT_FT2M] = 0.3048; - constantsValues[CONSTANT_PI] = 411557987.0 / 131002976.0; - constantsValues[CONSTANT_GRAVITY] = 9.80665; - constantsValues[CONSTANT_G] = 6.67408E-11; - constantsValues[CONSTANT_LB2KG] = 0.45359237; - constantsValues[CONSTANT_GAL_IMP2M3] = 0.00454609; + // These values are a hard-coded subset of unitConstants in the units + // 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. + // double constantsValues[CONSTANTS_COUNT]; + static const double constantsValues[CONSTANTS_COUNT] = { + [CONSTANT_FT2M] = 0.3048, // + [CONSTANT_PI] = 411557987.0 / 131002976.0, // + [CONSTANT_GRAVITY] = 9.80665, // + [CONSTANT_G] = 6.67408E-11, // + [CONSTANT_GAL_IMP2M3] = 0.00454609, // + [CONSTANT_LB2KG] = 0.45359237, // + }; for (int i = 0; i < CONSTANTS_COUNT; i++) { if (this->constants[i] == 0) { From df4b42f0b551fa0fcf33b2965f31d8f4bc48c367 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Wed, 15 Jul 2020 17:20:34 +0200 Subject: [PATCH 6/6] Add U_I18N_API to Factor class definition and method definitions. --- icu4c/source/i18n/unitconverter.cpp | 12 ++++++------ icu4c/source/i18n/unitconverter.h | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/icu4c/source/i18n/unitconverter.cpp b/icu4c/source/i18n/unitconverter.cpp index 2bb5ee3f141..8614a5e5b1c 100644 --- a/icu4c/source/i18n/unitconverter.cpp +++ b/icu4c/source/i18n/unitconverter.cpp @@ -18,7 +18,7 @@ U_NAMESPACE_BEGIN namespace units { -void Factor::multiplyBy(const Factor &rhs) { +void U_I18N_API Factor::multiplyBy(const Factor &rhs) { factorNum *= rhs.factorNum; factorDen *= rhs.factorDen; for (int i = 0; i < CONSTANTS_COUNT; i++) { @@ -31,7 +31,7 @@ void Factor::multiplyBy(const Factor &rhs) { offset = std::max(rhs.offset, offset); } -void Factor::divideBy(const Factor &rhs) { +void U_I18N_API Factor::divideBy(const Factor &rhs) { factorNum *= rhs.factorDen; factorDen *= rhs.factorNum; for (int i = 0; i < CONSTANTS_COUNT; i++) { @@ -44,7 +44,7 @@ void Factor::divideBy(const Factor &rhs) { offset = std::max(rhs.offset, offset); } -void Factor::power(int32_t power) { +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; @@ -62,7 +62,7 @@ void Factor::power(int32_t power) { } } -void Factor::flip() { +void U_I18N_API Factor::flip() { std::swap(factorNum, factorDen); for (int i = 0; i < CONSTANTS_COUNT; i++) { @@ -70,7 +70,7 @@ void Factor::flip() { } } -void Factor::applySiPrefix(UMeasureSIPrefix siPrefix) { +void U_I18N_API Factor::applySiPrefix(UMeasureSIPrefix siPrefix) { if (siPrefix == UMeasureSIPrefix::UMEASURE_SI_PREFIX_ONE) return; // No need to do anything double siApplied = std::pow(10.0, std::abs(siPrefix)); @@ -83,7 +83,7 @@ void Factor::applySiPrefix(UMeasureSIPrefix siPrefix) { factorNum *= siApplied; } -void Factor::substituteConstants() { +void U_I18N_API Factor::substituteConstants() { // These values are a hard-coded subset of unitConstants in the units // resources file. A unit test checks that all constants in the resource // file are at least recognised by the code. Derived constants' values or diff --git a/icu4c/source/i18n/unitconverter.h b/icu4c/source/i18n/unitconverter.h index cc85d924132..25967887e3d 100644 --- a/icu4c/source/i18n/unitconverter.h +++ b/icu4c/source/i18n/unitconverter.h @@ -36,7 +36,7 @@ typedef enum SigNum { } SigNum; /* Represents a conversion factor */ -struct Factor { +struct U_I18N_API Factor { double factorNum = 1; double factorDen = 1; double offset = 0;