fix unitConverter and add ConversionRates class

diff --git a/icu4c/source/i18n/unitconverter.cpp b/icu4c/source/i18n/unitconverter.cpp
index 7f2a0e48734aa55ddeb02a7993ce69e55cd45d40..ddb8d55d861301e08e488a9fb80aafbc1ed81f16 100644 (file)
--- 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<ConversionRateInfo> &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<ConversionRateInfo> &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<ConversionRateInfo> &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 8d13a7995cfc1558f77dd81707cdada3827a6d1e..a7c70c43e54f57e071643b054dd4b6096b0cd8e4 100644 (file)
--- 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> &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 5ad29217e3c5d67b3c3168c8736682c78ff096c8..c92e151604a33b9b2a892dc70a2274d5fc4c81ad 100644 (file)
--- a/icu4c/source/i18n/unitsdata.cpp
+++ b/icu4c/source/i18n/unitsdata.cpp
@@ -102,13 +102,14 @@ void U_I18N_API getAllConversionRates(MaybeStackVector<ConversionRateInfo> &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<ConversionRateInfo> U_I18N_API
-getConversionRatesInfo(const MaybeStackVector<MeasureUnit> &, UErrorCode &status) {
-    MaybeStackVector<ConversionRateInfo> 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 5f4306dc012b180fa12729986d3b7ef258b5f7a5..99383574c7b4020202dc4c91a6b347442f79025b 100644 (file)
--- 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<ConversionRateInfo> &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<ConversionRateInfo>
-    U_I18N_API getConversionRatesInfo(const MaybeStackVector<MeasureUnit> &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<ConversionRateInfo> conversionInfo_;
+};
 
 U_NAMESPACE_END
 

diff --git a/icu4c/source/test/depstest/dependencies.txt b/icu4c/source/test/depstest/dependencies.txt
index 6d3cebc57a614c87de52e9bbe9dca71a02b59908..4f693ae7b8bc4f3468caacdc6fc002c650be82a8 100644 (file)
--- 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 387d4565c4ff6a711c3b75fea952186f87b2250f..8944d0b0fe6ffa9eeee053256b45d066a04daa40 100644 (file)
--- 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<MeasureUnit> 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<MeasureUnit> 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<MeasureUnit> 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