From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com>
Date: Sat, 28 Mar 2020 19:56:51 +0000 (+0100)
Subject: Clean up getConversionRatesInfo to prepare for review.
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=dea77a18c488bb8ebb7a080a277f2e50d5fb99c1;p=icu

Clean up getConversionRatesInfo to prepare for review.
---

diff --git a/icu4c/source/i18n/unitsdata.cpp b/icu4c/source/i18n/unitsdata.cpp
index 2f751562122..81a9f49de45 100644
--- a/icu4c/source/i18n/unitsdata.cpp
+++ b/icu4c/source/i18n/unitsdata.cpp
@@ -56,6 +56,7 @@ class ConversionRateDataSink : public ResourceSink {
      * @param status The standard ICU error code output parameter.
      */
     void put(const char *source, ResourceValue &value, UBool /*noFallback*/, UErrorCode &status) {
+        if (U_FAILURE(status)) return;
         ResourceTable conversionRateTable = value.getTable(status);
         if (U_FAILURE(status)) return;
 
@@ -73,14 +74,16 @@ class ConversionRateDataSink : public ResourceSink {
                 offset = value.getString(lenOffset, status);
             }
         }
+        if (U_FAILURE(status)) return;
         if (baseUnit == NULL || factor == NULL) {
+            // We could not find a usable conversion rate.
             status = U_MISSING_RESOURCE_ERROR;
             return;
         }
 
         // Check if we already have the conversion rate in question.
         //
-        // TODO(revieW): We could do this skip-check *before* we fetch
+        // TODO(review): We could do this skip-check *before* we fetch
         // baseUnit/factor/offset based only on the source unit, but only if
         // we're certain we'll never get two different baseUnits for a given
         // source. This should be the case, since convertUnit entries in CLDR's
@@ -97,7 +100,6 @@ class ConversionRateDataSink : public ResourceSink {
                 return;
             }
         }
-        if (U_FAILURE(status)) return;
 
         // We don't have this ConversionRateInfo yet: add it.
         ConversionRateInfo *cr = outVector.emplaceBack();
@@ -242,12 +244,39 @@ void collectUnitPrefs(UResourceBundle *usageData, MaybeStackVector<UnitPreferenc
     }
 }
 
-// The input unit needs to be simple, but can have dimensionality != 1.
+/**
+ * Collects conversion information for a "single unit" (a unit whose complexity
+ * is UMEASURE_UNIT_SINGLE).
+ *
+ * This function currently only supports higher-dimensionality input units if
+ * they map to "single unit" output units. This means it don't support
+ * square-bar, one-per-bar, square-joule or one-per-joule. (Some unit types in
+ * this class: volume, consumption, torque, force, pressure, speed,
+ * acceleration, and more).
+ *
+ * TODO(hugovdm): maybe find and share (in documentation) a solid argument for
+ * why these kinds of input units won't be needed with higher dimensionality? Or
+ * start supporting them... Also: add unit tests demonstrating the
+ * U_ILLEGAL_ARGUMENT_ERROR returned for such units.
+ *
+ * @param unit The input unit. Its complexity must be UMEASURE_UNIT_SINGLE, but
+ * it may have a dimensionality != 1.
+ * @param converUnitsBundle A UResourceBundle instance for the convertUnits
+ * resource.
+ * @param convertSink The ConversionRateDataSink through which
+ * ConversionRateInfo instances are to be collected.
+ * @param baseSingleUnit Output parameter: if not NULL, the base unit through
+ * which conversion rates pivot to other similar units will be returned through
+ * this pointer.
+ * @param status The standard ICU error code output parameter.
+ */
 void processSingleUnit(const MeasureUnit &unit, const UResourceBundle *convertUnitsBundle,
                        ConversionRateDataSink &convertSink, MeasureUnit *baseSingleUnit,
                        UErrorCode &status) {
+    if (U_FAILURE(status)) return;
     int32_t dimensionality = unit.getDimensionality(status);
 
+    // Fetch the relevant entry in convertUnits.
     MeasureUnit simple = unit;
     if (dimensionality != 1 || simple.getSIPrefix(status) != UMEASURE_SI_PREFIX_ONE) {
         simple = unit.withDimensionality(1, status).withSIPrefix(UMEASURE_SI_PREFIX_ONE, status);
@@ -260,24 +289,14 @@ void processSingleUnit(const MeasureUnit &unit, const UResourceBundle *convertUn
         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:
+            // The baseUnit is a single unit, so can be raised to the
+            // dimensionality of the input unit.
             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;
         }
@@ -286,10 +305,12 @@ void processSingleUnit(const MeasureUnit &unit, const UResourceBundle *convertUn
 
 } // namespace
 
-MaybeStackVector<ConversionRateInfo> getConversionRatesInfo(const MeasureUnit source, const MeasureUnit target,
-                                                            MeasureUnit *baseCompoundUnit,
+MaybeStackVector<ConversionRateInfo> getConversionRatesInfo(const MeasureUnit source,
+                                                            const MeasureUnit target,
+                                                            MeasureUnit *baseUnit,
                                                             UErrorCode &status) {
     MaybeStackVector<ConversionRateInfo> result;
+    if (U_FAILURE(status)) return result;
 
     int32_t sourceUnitsLength, targetUnitsLength;
     LocalArray<MeasureUnit> sourceUnits = source.splitToSingleUnits(sourceUnitsLength, status);
@@ -298,8 +319,8 @@ MaybeStackVector<ConversionRateInfo> getConversionRatesInfo(const MeasureUnit so
     LocalUResourceBundlePointer unitsBundle(ures_openDirect(NULL, "units", &status));
     StackUResourceBundle convertUnitsBundle;
     ures_getByKey(unitsBundle.getAlias(), "convertUnits", convertUnitsBundle.getAlias(), &status);
-
     ConversionRateDataSink convertSink(result);
+
     MeasureUnit sourceBaseUnit;
     for (int i = 0; i < sourceUnitsLength; i++) {
         MeasureUnit baseUnit;
@@ -317,6 +338,7 @@ MaybeStackVector<ConversionRateInfo> getConversionRatesInfo(const MeasureUnit so
             sourceBaseUnit = sourceBaseUnit.product(baseUnit, status);
         }
     }
+
     MeasureUnit targetBaseUnit;
     for (int i = 0; i < targetUnitsLength; i++) {
         MeasureUnit baseUnit;
@@ -329,21 +351,30 @@ MaybeStackVector<ConversionRateInfo> getConversionRatesInfo(const MeasureUnit so
             }
             targetBaseUnit = baseUnit;
         } else {
-            // WIP/FIXME(hugovdm): I think I found a bug in targetBaseUnit.product():
-            // Target Base: <kilogram-square-meter-per-square-second> x <one-per-meter> => <meter>
+            // WIP/FIXME(hugovdm): ensure this gets fixed, then remove this
+            // comment: I think I found a bug in targetBaseUnit.product(). First
+            // symptom was an unexpected product, further exploration resulted
+            // in AddressSanitizer errors.
+            //
+            // The product was:
+            //
+            // <kilogram-square-meter-per-square-second> * <one-per-meter> => <meter>
+            //
+            // as output by a printf:
             //
-            // fprintf(stderr, "Target Base: <%s> x <%s> => ", targetBaseUnit.getIdentifier(),
+            // fprintf(stderr, "<%s> x <%s> => ",
+            //         targetBaseUnit.getIdentifier(),
             //         baseUnit.getIdentifier());
             targetBaseUnit = targetBaseUnit.product(baseUnit, status);
-            // fprintf(stderr, "<%s>\n", targetBaseUnit.getIdentifier());
-            // fprintf(stderr, "Status: %s\n", u_errorName(status));
+            // fprintf(stderr, "<%s> - Status: %s\n",
+            //         targetBaseUnit.getIdentifier(), u_errorName(status));
         }
     }
     if (targetBaseUnit != sourceBaseUnit) {
         status = U_ILLEGAL_ARGUMENT_ERROR;
         return result;
     }
-    if (baseCompoundUnit != NULL) { *baseCompoundUnit = sourceBaseUnit; }
+    if (baseUnit != NULL) { *baseUnit = sourceBaseUnit; }
     return result;
 }
 
diff --git a/icu4c/source/i18n/unitsdata.h b/icu4c/source/i18n/unitsdata.h
index 1437b686f8d..65752d194a0 100644
--- a/icu4c/source/i18n/unitsdata.h
+++ b/icu4c/source/i18n/unitsdata.h
@@ -18,6 +18,9 @@ U_NAMESPACE_BEGIN
 
 // Encapsulates "convertUnits" information from units resources, specifying how
 // to convert from one unit to another.
+//
+// Information in this class is still in the form of strings: symbolic constants
+// need to be interpreted.
 class U_I18N_API ConversionRateInfo {
   public:
     ConversionRateInfo(){};
@@ -30,10 +33,9 @@ class U_I18N_API ConversionRateInfo {
         this->offset.append(offset, status);
     };
     CharString sourceUnit;
-    CharString baseUnit; // FIXME/WIP: baseUnit
+    CharString baseUnit;
     CharString factor;
     CharString offset;
-    bool reciprocal = false;
 };
 
 // Encapsulates unitPreferenceData information from units resources, specifying
@@ -47,20 +49,21 @@ struct U_I18N_API UnitPreference {
 
 /**
  * Collects and returns ConversionRateInfo needed to convert from source to
- * baseUnit.
+ * baseUnit and from target to baseUnit.
  *
  * If source and target are not compatible for conversion, status will be set to
  * U_ILLEGAL_ARGUMENT_ERROR.
  *
  * @param source The source unit (the unit type converted from).
  * @param target The target unit (the unit type converted to).
- * @param baseCompoundUnit Output parameter: if not NULL, it will be set to the
- * compound base unit type used as pivot for converting from source to target.
+ * @param baseUnit Output parameter: if not NULL, it will be set to the base
+ * unit type used as pivot for converting from source to target. This may be a
+ * compound unit (a combination of base units).
  * @param status Receives status.
  */
 MaybeStackVector<ConversionRateInfo> U_I18N_API getConversionRatesInfo(MeasureUnit source,
                                                                        MeasureUnit target,
-                                                                       MeasureUnit *baseCompoundUnit,
+                                                                       MeasureUnit *baseUnit,
                                                                        UErrorCode &status);
 
 /**