From e0edd4416869fe6058a87839e0dafd89a183dc45 Mon Sep 17 00:00:00 2001
From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com>
Date: Tue, 11 Aug 2020 17:54:10 +0200
Subject: [PATCH 1/3] Explore Usage-related error codes, address
 icu-units/icu#36.

---
 icu4c/source/i18n/number_formatimpl.cpp       |  3 ++-
 icu4c/source/i18n/unitsdata.cpp               | 10 ++++++---
 icu4c/source/test/intltest/numbertest.h       |  1 +
 icu4c/source/test/intltest/numbertest_api.cpp | 22 +++++++++++++++++++
 4 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/icu4c/source/i18n/number_formatimpl.cpp b/icu4c/source/i18n/number_formatimpl.cpp
index 1480a907e64..ec03492fdbe 100644
--- a/icu4c/source/i18n/number_formatimpl.cpp
+++ b/icu4c/source/i18n/number_formatimpl.cpp
@@ -230,7 +230,8 @@ NumberFormatterImpl::macrosToMicroGenerator(const MacroProps& macros, bool safe,
     // Unit Preferences and Conversions as our first step
     if (macros.usage.isSet()) {
         if (!isCldrUnit) {
-            // We only support "usage" when the input unit is a CLDR Unit.
+            // We only support "usage" when the input unit is specified, and is
+            // a CLDR Unit.
             status = U_ILLEGAL_ARGUMENT_ERROR;
             return nullptr;
         }
diff --git a/icu4c/source/i18n/unitsdata.cpp b/icu4c/source/i18n/unitsdata.cpp
index 098c9f560bc..518c91aa13d 100644
--- a/icu4c/source/i18n/unitsdata.cpp
+++ b/icu4c/source/i18n/unitsdata.cpp
@@ -282,6 +282,8 @@ int32_t getPreferenceMetadataIndex(const MaybeStackVector<UnitPreferenceMetadata
     if (U_FAILURE(status)) { return -1; }
     if (idx >= 0) { return idx; }
     if (!foundCategory) {
+        // TODO(units): maybe add a unit test to ensure all units have a
+        // category and all categories are present in the data?
         status = U_ILLEGAL_ARGUMENT_ERROR;
         return -1;
     }
@@ -293,9 +295,9 @@ int32_t getPreferenceMetadataIndex(const MaybeStackVector<UnitPreferenceMetadata
         } else if (uprv_strcmp(desired.usage.data(), "default") != 0) {
             desired.usage.truncate(0).append("default", status);
         } else {
-            // TODO(icu-units/icu#36): reconsider consistency of errors.
-            // Currently this U_MISSING_RESOURCE_ERROR propagates when a
-            // U_NUMBER_SKELETON_SYNTAX_ERROR might be much more intuitive.
+            // "default" is not supposed to be missing for any valid category.
+            // TODO(units): maybe add a unit test to ensure every usage in the
+            // data file has a "default" entry?
             status = U_MISSING_RESOURCE_ERROR;
             return -1;
         }
@@ -310,6 +312,8 @@ int32_t getPreferenceMetadataIndex(const MaybeStackVector<UnitPreferenceMetadata
             idx = binarySearch(metadata, desired, &foundCategory, &foundUsage, &foundRegion, status);
         }
         if (!foundRegion) {
+            // TODO(units): maybe add a unit test to ensure every usage has a
+            // "001" region?
             status = U_MISSING_RESOURCE_ERROR;
             return -1;
         }
diff --git a/icu4c/source/test/intltest/numbertest.h b/icu4c/source/test/intltest/numbertest.h
index c96c329ad20..b5f52ccc553 100644
--- a/icu4c/source/test/intltest/numbertest.h
+++ b/icu4c/source/test/intltest/numbertest.h
@@ -55,6 +55,7 @@ class NumberFormatterApiTest : public IntlTestWithFieldPosition {
     void unitMeasure();
     void unitCompoundMeasure();
     void unitUsage();
+    void unitUsageErrorCodes();
     void unitCurrency();
     void unitPercent();
     void percentParity();
diff --git a/icu4c/source/test/intltest/numbertest_api.cpp b/icu4c/source/test/intltest/numbertest_api.cpp
index 85513b31399..4c50e33add0 100644
--- a/icu4c/source/test/intltest/numbertest_api.cpp
+++ b/icu4c/source/test/intltest/numbertest_api.cpp
@@ -76,6 +76,7 @@ void NumberFormatterApiTest::runIndexedTest(int32_t index, UBool exec, const cha
         TESTCASE_AUTO(unitMeasure);
         TESTCASE_AUTO(unitCompoundMeasure);
         TESTCASE_AUTO(unitUsage);
+        TESTCASE_AUTO(unitUsageErrorCodes);
         TESTCASE_AUTO(unitCurrency);
         TESTCASE_AUTO(unitPercent);
         if (!quick) {
@@ -750,6 +751,27 @@ void NumberFormatterApiTest::unitUsage() {
             u"0 ft");
 }
 
+void NumberFormatterApiTest::unitUsageErrorCodes() {
+    IcuTestErrorCode status(*this, "unitUsageErrorCodes()");
+    UnlocalizedNumberFormatter unloc_formatter;
+
+    unloc_formatter = NumberFormatter::forSkeleton(u"unit/foobar", status);
+    // This gives an error, because foobar is an invalid unit:
+    status.expectErrorAndReset(U_NUMBER_SKELETON_SYNTAX_ERROR);
+
+    unloc_formatter = NumberFormatter::forSkeleton(u"usage/foobar", status);
+    // This does not give an error, because usage is not looked up yet.
+    status.errIfFailureAndReset("Expected behaviour: no immediate error for invalid usage");
+    unloc_formatter.locale("en-GB").formatInt(1, status);
+    // Lacking a unit results in a failure. The skeleton is "incomplete", but we
+    // support adding the unit via the fluent API, so it is not an error until
+    // we build the formatting pipeline itself.
+    status.expectErrorAndReset(U_ILLEGAL_ARGUMENT_ERROR);
+    // Adding the unit as part of the fluent chain leads to success.
+    unloc_formatter.unit(MeasureUnit::getMeter()).locale("en-GB").formatInt(1, status);
+    status.assertSuccess();
+}
+
 void NumberFormatterApiTest::unitCompoundMeasure() {
     assertFormatDescending(
             u"Meters Per Second Short (unit that simplifies) and perUnit method",

From a3d5e3be0d6fdeca0e5630ef31c4d2607e85bab2 Mon Sep 17 00:00:00 2001
From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com>
Date: Tue, 11 Aug 2020 19:35:51 +0200
Subject: [PATCH 2/3] Remove three TODOs.

---
 icu4c/source/i18n/unitsdata.cpp | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/icu4c/source/i18n/unitsdata.cpp b/icu4c/source/i18n/unitsdata.cpp
index 518c91aa13d..702c09aaaae 100644
--- a/icu4c/source/i18n/unitsdata.cpp
+++ b/icu4c/source/i18n/unitsdata.cpp
@@ -282,8 +282,6 @@ int32_t getPreferenceMetadataIndex(const MaybeStackVector<UnitPreferenceMetadata
     if (U_FAILURE(status)) { return -1; }
     if (idx >= 0) { return idx; }
     if (!foundCategory) {
-        // TODO(units): maybe add a unit test to ensure all units have a
-        // category and all categories are present in the data?
         status = U_ILLEGAL_ARGUMENT_ERROR;
         return -1;
     }
@@ -296,8 +294,6 @@ int32_t getPreferenceMetadataIndex(const MaybeStackVector<UnitPreferenceMetadata
             desired.usage.truncate(0).append("default", status);
         } else {
             // "default" is not supposed to be missing for any valid category.
-            // TODO(units): maybe add a unit test to ensure every usage in the
-            // data file has a "default" entry?
             status = U_MISSING_RESOURCE_ERROR;
             return -1;
         }
@@ -312,8 +308,6 @@ int32_t getPreferenceMetadataIndex(const MaybeStackVector<UnitPreferenceMetadata
             idx = binarySearch(metadata, desired, &foundCategory, &foundUsage, &foundRegion, status);
         }
         if (!foundRegion) {
-            // TODO(units): maybe add a unit test to ensure every usage has a
-            // "001" region?
             status = U_MISSING_RESOURCE_ERROR;
             return -1;
         }

From 74e56cd147ec98d168452f07f88c39d23f654c39 Mon Sep 17 00:00:00 2001
From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com>
Date: Tue, 11 Aug 2020 20:37:48 +0200
Subject: [PATCH 3/3] Add a note about "001" being expected.

---
 icu4c/source/i18n/unitsdata.cpp | 1 +
 1 file changed, 1 insertion(+)

diff --git a/icu4c/source/i18n/unitsdata.cpp b/icu4c/source/i18n/unitsdata.cpp
index 702c09aaaae..d9f1015c94d 100644
--- a/icu4c/source/i18n/unitsdata.cpp
+++ b/icu4c/source/i18n/unitsdata.cpp
@@ -308,6 +308,7 @@ int32_t getPreferenceMetadataIndex(const MaybeStackVector<UnitPreferenceMetadata
             idx = binarySearch(metadata, desired, &foundCategory, &foundUsage, &foundRegion, status);
         }
         if (!foundRegion) {
+            // "001" is not supposed to be missing for any valid usage.
             status = U_MISSING_RESOURCE_ERROR;
             return -1;
         }