Code review feedback incorporated.

diff --git a/icu4c/source/i18n/unitsdata.cpp b/icu4c/source/i18n/unitsdata.cpp
index ba4066b55dc9f059015d2a57f4802d3771b01dc0..b215c3498ca3c34e2f8b3282e6f357d7e743b7a7 100644 (file)
--- a/icu4c/source/i18n/unitsdata.cpp
+++ b/icu4c/source/i18n/unitsdata.cpp
@@ -117,27 +117,23 @@ UnitPreferenceMetadata::UnitPreferenceMetadata(const char *category, const char
     this->prefsCount = prefsCount;
 }
 
-int32_t compareUnitPreferenceMetadata(const UnitPreferenceMetadata &a, const UnitPreferenceMetadata &b) {
-    int32_t cmp = uprv_strcmp(a.category.data(), b.category.data());
-    if (cmp == 0) { cmp = uprv_strcmp(a.usage.data(), b.usage.data()); }
-    if (cmp == 0) { cmp = uprv_strcmp(a.region.data(), b.region.data()); }
+int32_t UnitPreferenceMetadata::compareTo(const UnitPreferenceMetadata &other) const {
+    int32_t cmp = uprv_strcmp(category.data(), other.category.data());
+    if (cmp == 0) { cmp = uprv_strcmp(usage.data(), other.usage.data()); }
+    if (cmp == 0) { cmp = uprv_strcmp(region.data(), other.region.data()); }
     return cmp;
 }
 
-bool operator<(const UnitPreferenceMetadata &a, const UnitPreferenceMetadata &b) {
-    return compareUnitPreferenceMetadata(a, b) < 0;
-}
-
-int32_t compareUnitPreferenceMetadata(const UnitPreferenceMetadata &a, const UnitPreferenceMetadata &b,
-                                      bool *foundCategory, bool *foundUsage, bool *foundRegion) {
-    int32_t cmp = uprv_strcmp(a.category.data(), b.category.data());
+int32_t UnitPreferenceMetadata::compareTo(const UnitPreferenceMetadata &other, bool *foundCategory,
+                                          bool *foundUsage, bool *foundRegion) const {
+    int32_t cmp = uprv_strcmp(category.data(), other.category.data());
     if (cmp == 0) {
         *foundCategory = true;
-        cmp = uprv_strcmp(a.usage.data(), b.usage.data());
+        cmp = uprv_strcmp(usage.data(), other.usage.data());
     }
     if (cmp == 0) {
         *foundUsage = true;
-        cmp = uprv_strcmp(a.region.data(), b.region.data());
+        cmp = uprv_strcmp(region.data(), other.region.data());
     }
     if (cmp == 0) {
         *foundRegion = true;
@@ -145,6 +141,10 @@ int32_t compareUnitPreferenceMetadata(const UnitPreferenceMetadata &a, const Uni
     return cmp;
 }
 
+bool operator<(const UnitPreferenceMetadata &a, const UnitPreferenceMetadata &b) {
+    return a.compareTo(b) < 0;
+}
+
 /**
  * A ResourceSink that collects unit preferences information.
  *
@@ -172,7 +172,8 @@ class UnitPreferencesSink : public ResourceSink {
      * @param value The "unitPreferenceData" resource, containing unit
      * preferences data.
      * @param noFallback Ignored.
-     * @param status The standard ICU error code output parameter.
+     * @param status The standard ICU error code output parameter. Note: if an
+     * error is returned, outPrefs and outMetadata may be inconsistent.
      */
     void put(const char *key, ResourceValue &value, UBool /*noFallback*/, UErrorCode &status) {
         if (U_FAILURE(status)) { return; }
@@ -182,6 +183,10 @@ class UnitPreferencesSink : public ResourceSink {
             status = U_ILLEGAL_ARGUMENT_ERROR;
             return;
         }
+        // The unitPreferenceData structure (see data/misc/units.txt) contains a
+        // hierarchy of category/usage/region, within which are a set of
+        // preferences. Hence three for-loops and another loop for the
+        // preferences themselves:
         ResourceTable unitPreferenceDataTable = value.getTable(status);
         const char *category;
         for (int32_t i = 0; unitPreferenceDataTable.getKeyAndValue(i, category, value); i++) {
@@ -191,9 +196,13 @@ class UnitPreferencesSink : public ResourceSink {
                 ResourceTable regionTable = value.getTable(status);
                 const char *region;
                 for (int32_t k = 0; regionTable.getKeyAndValue(k, region, value); k++) {
+                    // `value` now contains the set of preferences for
+                    // category/usage/region.
                     ResourceArray unitPrefs = value.getArray(status);
                     if (U_FAILURE(status)) { return; }
                     int32_t prefLen = unitPrefs.getSize();
+
+                    // Update metadata for this set of preferences.
                     UnitPreferenceMetadata *meta = metadata->emplaceBack(
                         category, usage, region, preferences->length(), prefLen, status);
                     if (!meta) {
@@ -211,6 +220,7 @@ class UnitPreferencesSink : public ResourceSink {
                         }
                     }
 
+                    // Collect the individual preferences.
                     for (int32_t i = 0; unitPrefs.getValue(i, value); i++) {
                         UnitPreference *up = preferences->emplaceBack();
                         if (!up) {
@@ -261,8 +271,7 @@ int32_t binarySearch(const MaybeStackVector<UnitPreferenceMetadata> *metadata, c
     *foundRegion = false;
     while (start < end) {
         int32_t mid = (start + end) / 2;
-        int32_t cmp = compareUnitPreferenceMetadata(*(*metadata)[mid], desired, foundCategory,
-                                                    foundUsage, foundRegion);
+        int32_t cmp = (*metadata)[mid]->compareTo(desired, foundCategory, foundUsage, foundRegion);
         if (cmp < 0) {
             start = mid + 1;
         } else if (cmp > 0) {
@@ -294,9 +303,10 @@ int32_t binarySearch(const MaybeStackVector<UnitPreferenceMetadata> *metadata, c
  * @param region The region for which preferences are needed. If there are no
  * region-specific preferences, this function automatically falls back to the
  * "001" region (global).
- * @param status The standard ICU error code output parameter. If an invalid
- * category is given, status will be U_ILLEGAL_ARGUMENT_ERROR. If fallback to
- * "default" or "001" didn't resolve, status will be U_MISSING_RESOURCE.
+ * @param status The standard ICU error code output parameter.
+ *   * If an invalid category is given, status will be U_ILLEGAL_ARGUMENT_ERROR.
+ *   * If fallback to "default" or "001" didn't resolve, status will be
+ *     U_MISSING_RESOURCE.
  * @return The index into the metadata vector which represents the appropriate
  * preferences. If appropriate preferences are not found, -1 is returned.
  */
@@ -370,16 +380,19 @@ U_I18N_API UnitPreferences::UnitPreferences(UErrorCode &status) {
     ures_getAllItemsWithFallback(unitsBundle.getAlias(), "unitPreferenceData", sink, status);
 }
 
-// TODO/WIP: make outPrefernces const, make function const, propagate const as needed.
-// TODO/WIP: create a simpler class to replace `UnitPreference **&outPrefrences`.
+// TODO: make outPreferences const?
+//
+// TODO: consider replacing `UnitPreference **&outPrefrences` with slice class
+// of some kind.
 void U_I18N_API UnitPreferences::getPreferencesFor(const char *category, const char *usage,
-                                                   const char *region, UnitPreference **&outPreferences,
+                                                   const char *region,
+                                                   const UnitPreference **&outPreferences,
                                                    int32_t &preferenceCount, UErrorCode &status) const {
     int32_t idx = getPreferenceMetadataIndex(&metadata_, category, usage, region, status);
     if (U_FAILURE(status)) { return; }
     U_ASSERT(idx >= 0); // Failures should have been taken care of by `status`.
     const UnitPreferenceMetadata *m = metadata_[idx];
-    outPreferences = unitPrefs_.getAlias() + m->prefsOffset;
+    outPreferences = const_cast<const UnitPreference **>(unitPrefs_.getAlias()) + m->prefsOffset;
     preferenceCount = m->prefsCount;
 }
 

diff --git a/icu4c/source/i18n/unitsdata.h b/icu4c/source/i18n/unitsdata.h
index cf4f580a1576302725114adc74caa329add5da14..173cc519425b7007cc9357febb9c60da7b195c15 100644 (file)
--- a/icu4c/source/i18n/unitsdata.h
+++ b/icu4c/source/i18n/unitsdata.h
@@ -93,7 +93,8 @@ namespace {
  * UnitPreferenceMetadata lives in the anonymous namespace, because it should
  * only be useful to internal code and unit testing code.
  */
-struct U_I18N_API UnitPreferenceMetadata : public UMemory {
+class U_I18N_API UnitPreferenceMetadata : public UMemory {
+  public:
     UnitPreferenceMetadata(){};
     UnitPreferenceMetadata(const char *category, const char *usage, const char *region,
                            int32_t prefsOffset, int32_t prefsCount, UErrorCode &status);
@@ -112,6 +113,10 @@ struct U_I18N_API UnitPreferenceMetadata : public UMemory {
     int32_t prefsOffset;
     // The number of preferences that form this set.
     int32_t prefsCount;
+
+    int32_t compareTo(const UnitPreferenceMetadata &other) const;
+    int32_t compareTo(const UnitPreferenceMetadata &other, bool *foundCategory, bool *foundUsage,
+                      bool *foundRegion) const;
 };
 
 } // namespace
@@ -152,13 +157,10 @@ class U_I18N_API UnitPreferences {
      * @param outPreferences The vector to which preferences will be added.
      * @param status Receives status.
      *
-     * - TODO/WIP: make outPrefernces const, make function const, propagate
-     *   const as needed.
-     * - TODO/WIP: create a simpler class to replace `UnitPreference
-     *   **&outPrefrences`.
+     * TODO: maybe replace `UnitPreference **&outPrefrences` with a slice class?
      */
     void getPreferencesFor(const char *category, const char *usage, const char *region,
-                           UnitPreference **&outPreferences, int32_t &preferenceCount,
+                           const UnitPreference **&outPreferences, int32_t &preferenceCount,
                            UErrorCode &status) const;
 
   protected:

diff --git a/icu4c/source/test/intltest/unitsdatatest.cpp b/icu4c/source/test/intltest/unitsdatatest.cpp
index dad34a16b6345b27de485841e43247a451f87d70..f3e2eef0d845b4672d35f67fba6831f181c64f46 100644 (file)
--- a/icu4c/source/test/intltest/unitsdatatest.cpp
+++ b/icu4c/source/test/intltest/unitsdatatest.cpp
@@ -89,25 +89,9 @@ void UnitsDataTest::testGetPreferences() {
     assertTrue(UnicodeString("Preferences count: ") + unitPrefs->length() + " > 250",
                unitPrefs->length() > 250);
 
-    // Dump all preferences... TODO/WIP: remove whole block? This was just
-    // debugging/development output.
-    logln("Unit Preferences:");
-    for (int32_t i = 0; i < metadata->length(); i++) {
-        logln("%d: category %s, usage %s, region %s, offset %d, count %d", i,
-              (*metadata)[i]->category.data(), (*metadata)[i]->usage.data(),
-              (*metadata)[i]->region.data(), (*metadata)[i]->prefsOffset, (*metadata)[i]->prefsCount);
-        int32_t offset = (*metadata)[i]->prefsOffset;
-        int32_t count = (*metadata)[i]->prefsCount;
-        for (int32_t j = offset; j < offset + count; j++) {
-            auto p = (*unitPrefs)[j];
-            logln("  %d: 0x%x unit %s, geq %f, skeleton \"%s\"", j, p, p->unit.data(), p->geq,
-                  p->skeleton.data());
-        }
-    }
-
     for (const auto &t : testCases) {
         logln(t.name);
-        UnitPreference **prefs;
+        const UnitPreference **prefs;
         int32_t prefsCount;
         preferences.getPreferencesFor(t.category, t.usage, t.region, prefs, prefsCount, status);
         if (status.errIfFailureAndReset("getPreferencesFor(\"%s\", \"%s\", \"%s\", ...", t.category,