From: Younies Mahmoud Date: Tue, 28 Apr 2020 15:31:41 +0000 (+0200) Subject: fix unitConverter and add ConversionRates class X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=f53c7f28e153e74ca8d3788e2072ca365ac75b0d;p=icu fix unitConverter and add ConversionRates class --- diff --git a/icu4c/source/i18n/unitconverter.cpp b/icu4c/source/i18n/unitconverter.cpp index 7f2a0e48734..ddb8d55d861 100644 --- a/icu4c/source/i18n/unitconverter.cpp +++ b/icu4c/source/i18n/unitconverter.cpp @@ -15,26 +15,11 @@ U_NAMESPACE_BEGIN namespace { - -const ConversionRateInfo * -extractConversionInfo(StringPiece source, - const MaybeStackVector &conversionRateInfoList, - UErrorCode &status) { - for (size_t i = 0, n = conversionRateInfoList.length(); i < n; ++i) { - if (conversionRateInfoList[i]->sourceUnit.toStringPiece() == source) - return conversionRateInfoList[i]; - } - - status = U_INTERNAL_PROGRAM_ERROR; - return nullptr; -} - /** * Extracts the compound base unit of a compound unit (`source`). For example, if the source unit is * `square-mile-per-hour`, the compound base unit will be `square-meter-per-second` */ -MeasureUnit extractCompoundBaseUnit(const MeasureUnit &source, - const MaybeStackVector &conversionRateInfoList, +MeasureUnit extractCompoundBaseUnit(const MeasureUnit &source, const ConversionRates &conversionRates, UErrorCode &status) { MeasureUnit result; int32_t count; @@ -46,8 +31,7 @@ MeasureUnit extractCompoundBaseUnit(const MeasureUnit &source, // Extract `ConversionRateInfo` using the absolute unit. For example: in case of `square-meter`, // we will use `meter` const auto singleUnitImpl = SingleUnitImpl::forMeasureUnit(singleUnit, status); - const auto rateInfo = - extractConversionInfo(singleUnitImpl.identifier, conversionRateInfoList, status); + const auto rateInfo = conversionRates.extractConversionInfo(singleUnitImpl.identifier, status); if (U_FAILURE(status)) return result; if (rateInfo == nullptr) { status = U_INTERNAL_PROGRAM_ERROR; @@ -61,11 +45,11 @@ MeasureUnit extractCompoundBaseUnit(const MeasureUnit &source, int32_t baseUnitsCount; auto baseUnits = compoundBaseUnit.splitToSingleUnits(baseUnitsCount, status); for (int j = 0; j < baseUnitsCount; j++) { - result = - result.product(baseUnits[j].withDimensionality(baseUnits[j].getDimensionality(status) * - singleUnit.getDimensionality(status), - status), - status); + int8_t newDimensionality = + baseUnits[j].getDimensionality(status) * singleUnit.getDimensionality(status); + result = result.product(baseUnits[j].withDimensionality(newDimensionality, status), status); + + if (U_FAILURE(status)) { return result; } } } @@ -74,11 +58,12 @@ MeasureUnit extractCompoundBaseUnit(const MeasureUnit &source, } // namespace -UnitsConvertibilityState U_I18N_API checkConvertibility( - const MeasureUnit &source, const MeasureUnit &target, - const MaybeStackVector &conversionRateInfoList, UErrorCode &status) { - auto sourceBaseUnit = extractCompoundBaseUnit(source, conversionRateInfoList, status); - auto targetBaseUnit = extractCompoundBaseUnit(target, conversionRateInfoList, status); +UnitsConvertibilityState U_I18N_API checkConvertibility(const MeasureUnit &source, + const MeasureUnit &target, + const ConversionRates &conversionRates, + UErrorCode &status) { + auto sourceBaseUnit = extractCompoundBaseUnit(source, conversionRates, status); + auto targetBaseUnit = extractCompoundBaseUnit(target, conversionRates, status); if (U_FAILURE(status)) return UNCONVERTIBLE; diff --git a/icu4c/source/i18n/unitconverter.h b/icu4c/source/i18n/unitconverter.h index 8d13a7995cf..a7c70c43e54 100644 --- a/icu4c/source/i18n/unitconverter.h +++ b/icu4c/source/i18n/unitconverter.h @@ -21,9 +21,10 @@ enum U_I18N_API UnitsConvertibilityState { UNCONVERTIBLE, }; -UnitsConvertibilityState U_I18N_API -checkConvertibility(const MeasureUnit &source, const MeasureUnit &target, - const MaybeStackVector &conversionRateInfo, UErrorCode &status); +UnitsConvertibilityState U_I18N_API checkConvertibility(const MeasureUnit &source, + const MeasureUnit &target, + const ConversionRates &conversionRates, + UErrorCode &status); U_NAMESPACE_END diff --git a/icu4c/source/i18n/unitsdata.cpp b/icu4c/source/i18n/unitsdata.cpp index 5ad29217e3c..c92e151604a 100644 --- a/icu4c/source/i18n/unitsdata.cpp +++ b/icu4c/source/i18n/unitsdata.cpp @@ -102,13 +102,14 @@ void U_I18N_API getAllConversionRates(MaybeStackVector &resu ures_getAllItemsWithFallback(unitsBundle.getAlias(), "convertUnits", sink, status); } -// TODO(hugovdm): ensure this gets removed. Currently -// https://github.com/sffc/icu/pull/32 is making use of it. -MaybeStackVector U_I18N_API -getConversionRatesInfo(const MaybeStackVector &, UErrorCode &status) { - MaybeStackVector result; - getAllConversionRates(result, status); - return result; +const ConversionRateInfo *ConversionRates::extractConversionInfo(StringPiece source, + UErrorCode &status) const { + for (size_t i = 0, n = conversionInfo_.length(); i < n; ++i) { + if (conversionInfo_[i]->sourceUnit.toStringPiece() == source) return conversionInfo_[i]; + } + + status = U_INTERNAL_PROGRAM_ERROR; + return nullptr; } U_NAMESPACE_END diff --git a/icu4c/source/i18n/unitsdata.h b/icu4c/source/i18n/unitsdata.h index 5f4306dc012..99383574c7b 100644 --- a/icu4c/source/i18n/unitsdata.h +++ b/icu4c/source/i18n/unitsdata.h @@ -49,16 +49,28 @@ class U_I18N_API ConversionRateInfo : public UMemory { void U_I18N_API getAllConversionRates(MaybeStackVector &result, UErrorCode &status); /** - * Temporary backward-compatibility function. - * - * TODO(hugovdm): ensure this gets removed. Currently - * https://github.com/sffc/icu/pull/32 is making use of it. - * - * @param units Ignored. - * @return the result of getAllConversionRates. + * Contains all the supported conversion rates. */ -MaybeStackVector - U_I18N_API getConversionRatesInfo(const MaybeStackVector &units, UErrorCode &status); +class U_I18N_API ConversionRates { + public: + /** + * Constructor + * + * @param status Receives status. + */ + ConversionRates(UErrorCode &status) { getAllConversionRates(conversionInfo_, status); } + + /** + * Returns a pointer to the conversion rate info that match the `source`. + * + * @param source Contains the source. + * @param status Receives status. + */ + const ConversionRateInfo *extractConversionInfo(StringPiece source, UErrorCode &status) const; + + private: + MaybeStackVector conversionInfo_; +}; U_NAMESPACE_END diff --git a/icu4c/source/test/depstest/dependencies.txt b/icu4c/source/test/depstest/dependencies.txt index 6d3cebc57a6..4f693ae7b8b 100644 --- a/icu4c/source/test/depstest/dependencies.txt +++ b/icu4c/source/test/depstest/dependencies.txt @@ -1063,7 +1063,7 @@ group: sharedbreakiterator breakiterator group: units_extra - measunit_extra.o unitconverter.o + measunit_extra.o deps units ucharstriebuilder ucharstrie uclean_i18n @@ -1073,7 +1073,7 @@ group: units stringenumeration errorcode group: unitsformatter - unitsdata.o + unitsdata.o unitconverter.o deps resourcebundle diff --git a/icu4c/source/test/intltest/unitstest.cpp b/icu4c/source/test/intltest/unitstest.cpp index 387d4565c4f..8944d0b0fe6 100644 --- a/icu4c/source/test/intltest/unitstest.cpp +++ b/icu4c/source/test/intltest/unitstest.cpp @@ -27,7 +27,7 @@ class UnitsTest : public IntlTest { void runIndexedTest(int32_t index, UBool exec, const char *&name, char *par = NULL); void testConversionCapability(); - // void testConversions(); // TODO(hugo): it doesnot pass. + void testConversions(); void testPreferences(); // void testBasic(); // void testSiPrefixes(); @@ -75,6 +75,7 @@ void UnitsTest::testConversionCapability() { {"kilometer-per-second", "foot-per-second", CONVERTIBLE}, // {"square-hectare", "p4-foot", CONVERTIBLE}, // {"square-kilometer-per-second", "second-per-square-meter", RECIPROCAL}, // + // TODO: Remove the following test cases after hocking up unitsTest.txt. {"g-force", "meter-per-square-second", CONVERTIBLE}, // {"ohm", "kilogram-square-meter-per-cubic-second-square-ampere", CONVERTIBLE}, // {"electronvolt", "kilogram-square-meter-per-square-second", CONVERTIBLE}, // @@ -123,12 +124,8 @@ void UnitsTest::testConversionCapability() { MeasureUnit source = MeasureUnit::forIdentifier(testCase.source, status); MeasureUnit target = MeasureUnit::forIdentifier(testCase.target, status); - MaybeStackVector units; - units.emplaceBack(source); - units.emplaceBack(target); - - const auto &conversionRateInfoList = getConversionRatesInfo(units, status); - auto convertibility = checkConvertibility(source, target, conversionRateInfoList, status); + ConversionRates conversionRates(status); + auto convertibility = checkConvertibility(source, target, conversionRates, status); assertEquals("Conversion Capability", testCase.expectedState, convertibility); } @@ -274,7 +271,7 @@ StringPiece trimField(char *(&field)[2]) { */ void unitsTestDataLineFn(void *context, char *fields[][2], int32_t fieldCount, UErrorCode *pErrorCode) { if (U_FAILURE(*pErrorCode)) return; - UnitsTest *unitsTest = (UnitsTest *)context; + UnitsTest* unitsTest = (UnitsTest*)context; (void)fieldCount; // unused UParseLineFn variable IcuTestErrorCode status(*unitsTest, "unitsTestDatalineFn"); @@ -301,27 +298,29 @@ void unitsTestDataLineFn(void *context, char *fields[][2], int32_t fieldCount, U quantity.length(), quantity.data(), x.length(), x.data(), y.length(), y.data(), expected, commentConversionFormula.length(), commentConversionFormula.data()); - // Convertibility: - MaybeStackVector units; - units.emplaceBack(sourceUnit); - units.emplaceBack(targetUnit); - const auto &conversionRateInfoList = getConversionRatesInfo(units, status); - if (status.errIfFailureAndReset("getConversionRatesInfo(...)")) return; + // WIP(hugovdm): hook this up to actual tests. + + // // Convertibility: + // MaybeStackVector units; + // units.emplaceBack(sourceUnit); + // units.emplaceBack(targetUnit); + // const auto &conversionRateInfoList = getConversionRatesInfo(units, status); + // if (status.errIfFailureAndReset("getConversionRatesInfo(...)")) return; - auto actualState = checkConvertibility(sourceUnit, targetUnit, conversionRateInfoList, status); - if (status.errIfFailureAndReset("checkConvertibility(<%s>, <%s>, ...)", sourceUnit.getIdentifier(), - targetUnit.getIdentifier())) { - return; - } + // auto actualState = checkUnitsState(sourceUnit, targetUnit, conversionRateInfoList, status); + // if (status.errIfFailureAndReset("checkUnitsState(<%s>, <%s>, ...)", sourceUnit.getIdentifier(), + // targetUnit.getIdentifier())) { + // return; + // } - CharString msg; - msg.append("convertible: ", status) - .append(sourceUnit.getIdentifier(), status) - .append(" -> ", status) - .append(targetUnit.getIdentifier(), status); - if (status.errIfFailureAndReset("msg construction")) return; + // CharString msg; + // msg.append("convertible: ", status) + // .append(sourceUnit.getIdentifier(), status) + // .append(" -> ", status) + // .append(targetUnit.getIdentifier(), status); + // if (status.errIfFailureAndReset("msg construction")) return; - unitsTest->assertTrue(msg.data(), actualState != UNCONVERTIBLE); + // unitsTest->assertTrue(msg.data(), actualState != UNCONVERTIBLE); // Unit conversion... untested: // UnitConverter converter(sourceUnit, targetUnit, status); @@ -329,32 +328,31 @@ void unitsTestDataLineFn(void *context, char *fields[][2], int32_t fieldCount, U // unitsTest->assertEqualsNear(quantity.data(), expected, got, 0.0001); } -// /** -// * Runs data-driven unit tests for unit conversion. It looks for the test cases -// * in source/test/testdata/units/unitsTest.txt, which originates in CLDR. -// */ -// void UnitsTest::testConversions() { -// const char *filename = "unitsTest.txt"; -// const int32_t kNumFields = 5; -// char *fields[kNumFields][2]; - -// IcuTestErrorCode errorCode(*this, "UnitsTest::testConversions"); -// const char *sourceTestDataPath = getSourceTestData(errorCode); -// if (errorCode.errIfFailureAndReset("unable to find the source/test/testdata " -// "folder (getSourceTestData())")) { -// return; -// } +/** + * Runs data-driven unit tests for unit conversion. It looks for the test cases + * in source/test/testdata/units/unitsTest.txt, which originates in CLDR. + */ +void UnitsTest::testConversions() { + const char *filename = "unitsTest.txt"; + const int32_t kNumFields = 5; + char *fields[kNumFields][2]; + + IcuTestErrorCode errorCode(*this, "UnitsTest::testConversions"); + const char *sourceTestDataPath = getSourceTestData(errorCode); + if (errorCode.errIfFailureAndReset("unable to find the source/test/testdata " + "folder (getSourceTestData())")) { + return; + } -// CharString path(sourceTestDataPath, errorCode); -// path.appendPathPart("units", errorCode); -// path.appendPathPart(filename, errorCode); + CharString path(sourceTestDataPath, errorCode); + path.appendPathPart("units", errorCode); + path.appendPathPart(filename, errorCode); -// u_parseDelimitedFile(path.data(), ';', fields, kNumFields, unitsTestDataLineFn, this, errorCode); -// if (errorCode.errIfFailureAndReset("error parsing %s: %s\n", path.data(), u_errorName(errorCode))) -// { -// return; -// } -// } + u_parseDelimitedFile(path.data(), ';', fields, kNumFields, unitsTestDataLineFn, this, errorCode); + if (errorCode.errIfFailureAndReset("error parsing %s: %s\n", path.data(), u_errorName(errorCode))) { + return; + } +} /** * This class represents the output fields from unitPreferencesTest.txt. Please