From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com>
Date: Thu, 26 Mar 2020 14:29:44 +0000 (+0100)
Subject: Fix for base unit complexity != 1. Eg: volume -> cubic-meter.
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=ec0227643ebfc96da3b4f4c5c3abec6e1bb10867;p=icu

Fix for base unit complexity != 1. Eg: volume -> cubic-meter.
---

diff --git a/icu4c/source/i18n/getunitsdata.cpp b/icu4c/source/i18n/getunitsdata.cpp
index b98d119e171..ffb0415807e 100644
--- a/icu4c/source/i18n/getunitsdata.cpp
+++ b/icu4c/source/i18n/getunitsdata.cpp
@@ -242,6 +242,7 @@ void collectUnitPrefs(UResourceBundle *usageData, MaybeStackVector<UnitPreferenc
     }
 }
 
+// The input unit needs to be simple, but can have dimensionality != 1.
 void processSingleUnit(const MeasureUnit &unit, const UResourceBundle *convertUnitsBundle,
                        ConversionRateDataSink &convertSink, MeasureUnit *baseSingleUnit,
                        UErrorCode &status) {
@@ -254,7 +255,32 @@ void processSingleUnit(const MeasureUnit &unit, const UResourceBundle *convertUn
     ures_getAllItemsWithFallback(convertUnitsBundle, simple.getIdentifier(), convertSink, status);
 
     if (baseSingleUnit != NULL) {
-        *baseSingleUnit = convertSink.getLastBaseUnit(status).withDimensionality(dimensionality, status);
+        MeasureUnit baseUnit = convertSink.getLastBaseUnit(status);
+
+        if (dimensionality == 1) {
+            *baseSingleUnit = baseUnit;
+        } else if (baseUnit.getComplexity(status) == UMEASURE_UNIT_SINGLE) {
+            // TODO(hugovdm): find examples where we're converting a *-per-* to
+            // a square-*? Does one ever square frequency? What about
+            // squared-speed in the case of mv^2? Or F=ma^2?
+            //
+            // baseUnit might also have dimensionality, e.g. cubic-meter -
+            // retain this instead of overriding with input unit dimensionality:
+            dimensionality *= baseUnit.getDimensionality(status);
+            *baseSingleUnit = baseUnit.withDimensionality(dimensionality, status);
+        } else {
+            // We only support higher dimensionality input units if they map to
+            // simple base units, such that that base unit can have the
+            // dimensionality easily applied.
+            //
+            // TODO(hugovdm): produce succeeding examples of simple input unit
+            // mapped to a different simple target/base unit.
+            //
+            // TODO(hugovdm): produce failing examples of higher-dimensionality
+            // or inverted input units that map to compound output units.
+            status = U_ILLEGAL_ARGUMENT_ERROR;
+            return;
+        }
     }
 }
 
@@ -290,7 +316,6 @@ MaybeStackVector<ConversionRateInfo> getConversionRatesInfo(const MeasureUnit so
         }
     }
     if (baseCompoundUnit != NULL) {
-        fprintf(stderr, "source base: %s\n", baseCompoundUnit->getIdentifier());
         *baseCompoundUnit = MeasureUnit();
     }
     for (int i = 0; i < targetUnitsLength; i++) {
diff --git a/icu4c/source/test/intltest/unitstest.cpp b/icu4c/source/test/intltest/unitstest.cpp
index d53e6e8539a..b5a5e83f0a5 100644
--- a/icu4c/source/test/intltest/unitstest.cpp
+++ b/icu4c/source/test/intltest/unitstest.cpp
@@ -590,10 +590,16 @@ void UnitsTest::testGetConversionRateInfo() {
         {"centimeter-per-square-milligram",
          "inch-per-square-ounce",
          {"pound", "stone", "ton"},
-         "kilogram"},
+         "meter-per-square-kilogram"},
         {"liter", "gallon", {"liter", "gallon", NULL}, "cubic-meter"},
         {"stone-and-pound", "ton", {"pound", "stone", "ton"}, "kilogram"},
         {"mile-per-hour", "dekameter-per-hour", {"mile", "hour", "meter"}, "meter-per-second"},
+        {"kilovolt-ampere",
+         "horsepower",
+         {"volt", "ampere", "horsepower"},
+         "kilogram-square-meter-per-cubic-second"}, // watt
+        // TODO: include capacitance test case with base unit:
+        // pow4-second-square-ampere-per-kilogram-square-meter;
     };
     for (const auto &t : testCases) {
         logln("---testing: source=\"%s\", target=\"%s\", expectedBaseUnit=\"%s\"", t.sourceUnit,
@@ -605,8 +611,13 @@ void UnitsTest::testGetConversionRateInfo() {
         MeasureUnit targetUnit = MeasureUnit::forIdentifier(t.targetUnit, status);
         MaybeStackVector<ConversionRateInfo> conversionInfo =
             getConversionRatesInfo(sourceUnit, targetUnit, &baseCompoundUnit, status);
+        if (status.errIfFailureAndReset("getConversionRatesInfo(<%s>, <%s>, ...)",
+                                        sourceUnit.getIdentifier(), targetUnit.getIdentifier())) {
+            continue;
+        }
 
-        logln("---found BaseUnit=\"%s\"", baseCompoundUnit.getIdentifier());
+        assertEquals("baseCompoundUnit returned by getConversionRatesInfo", t.baseUnit,
+                     baseCompoundUnit.getIdentifier());
         for (int i = 0; i < conversionInfo.length(); i++) {
             ConversionRateInfo *cri;
             cri = conversionInfo[i];