From 2fc5cc2df0b7fe88f68363c713bad884732837bc Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Wed, 27 May 2020 20:04:36 +0200 Subject: [PATCH 1/3] Add test cases for dropping last component(s) of usage. --- icu4c/source/test/intltest/unitsdatatest.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/icu4c/source/test/intltest/unitsdatatest.cpp b/icu4c/source/test/intltest/unitsdatatest.cpp index 64e8c0a2583..4e44e47d0c3 100644 --- a/icu4c/source/test/intltest/unitsdatatest.cpp +++ b/icu4c/source/test/intltest/unitsdatatest.cpp @@ -103,6 +103,10 @@ void UnitsDataTest::testGetPreferences() { {"XX default falls back to 001", "length", "default", "XX", WorldLenMax, WorldLenMin}, {"Unknown usage US", "length", "foobar", "US", USLenMax, USLenMin}, {"Unknown usage 001", "length", "foobar", "XX", WorldLenMax, WorldLenMin}, + {"Fallback", "length", "person-height-xyzzy", "DE", "meter-and-centimeter", + "meter-and-centimeter"}, + {"Fallback twice", "length", "person-height-xyzzy-foo", "DE", "meter-and-centimeter", + "meter-and-centimeter"}, }; IcuTestErrorCode status(*this, "testGetPreferences"); UnitPreferencesOpenedUp preferences(status); From 9b2f8d33c0628ae6ef2131340d3d2959a6edf0de Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Thu, 28 May 2020 00:09:55 +0200 Subject: [PATCH 2/3] Support usage component dropping, and more: - Improve efficiency by not constructing UnitPreferenceMetadata instance in binarySearch: reuse an instance passed in. - Fix stale documentation. (Doc freshness is hard! ;-) - getPreferenceMetadataIndex actually returns -1 upon failure. --- icu4c/source/i18n/unitsdata.cpp | 55 ++++++++++++++------------------- icu4c/source/i18n/unitsdata.h | 18 ++++++----- 2 files changed, 34 insertions(+), 39 deletions(-) diff --git a/icu4c/source/i18n/unitsdata.cpp b/icu4c/source/i18n/unitsdata.cpp index e80a6950678..1052d1080f2 100644 --- a/icu4c/source/i18n/unitsdata.cpp +++ b/icu4c/source/i18n/unitsdata.cpp @@ -259,10 +259,9 @@ class UnitPreferencesSink : public ResourceSink { MaybeStackVector *metadata; }; -int32_t binarySearch(const MaybeStackVector *metadata, const char *category, - const char *usage, const char *region, bool *foundCategory, bool *foundUsage, +int32_t binarySearch(const MaybeStackVector *metadata, + const UnitPreferenceMetadata &desired, bool *foundCategory, bool *foundUsage, bool *foundRegion, UErrorCode &status) { - UnitPreferenceMetadata desired(category, usage, region, -1, -1, status); if (U_FAILURE(status)) { return -1; } int32_t start = 0; int32_t end = metadata->length(); @@ -285,21 +284,15 @@ int32_t binarySearch(const MaybeStackVector *metadata, c /** * Finds the UnitPreferenceMetadata instance that matches the given category, - * usage and region: if missing, region falls back to "001", and usage falls - * back to "default". - * - * This is implemented as a binary search, with fallback restarting the search - * from the search range at which the parent in the category/usage/region - * hierarchy was found. + * usage and region: if missing, region falls back to "001", and usage + * repeatedly drops tailing components, eventually trying "default" + * ("land-agriculture-grain" -> "land-agriculture" -> "land" -> "default"). * * @param metadata The full list of UnitPreferenceMetadata instances. - * @param category The category to search for. If category is not known, it can - * be resolved from the baseunit of the input (for supported unit categories). - * TODO(hugovdm): implement the unit->category lookup (via "unitQuantities" in - * the units resource bundle). + * @param category The category to search for. See getUnitCategory(). * @param usage The usage for which formatting preferences is needed. If the - * given usage is not known, this function automatically falls back to "default" - * usage. + * given usage is not known, automatic fallback occurs, see function description + * above. * @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). @@ -315,37 +308,37 @@ int32_t getPreferenceMetadataIndex(const MaybeStackVector= 0) { return idx; } if (!foundCategory) { status = U_ILLEGAL_ARGUMENT_ERROR; - return idx; + return -1; } U_ASSERT(foundCategory); - if (!foundUsage) { - if (uprv_strcmp(usage, "default") != 0) { - usage = "default"; - idx = binarySearch(metadata, category, usage, region, &foundCategory, &foundUsage, - &foundRegion, status); - } - if (!foundUsage) { + while (!foundUsage) { + int32_t lastDashIdx = desired.usage.lastIndexOf('-'); + if (lastDashIdx > 0) { + desired.usage.truncate(lastDashIdx); + } else if (uprv_strcmp(desired.usage.data(), "default") != 0) { + desired.usage.truncate(0).append("default", status); + } else { status = U_MISSING_RESOURCE_ERROR; - return idx; + return -1; } + idx = binarySearch(metadata, desired, &foundCategory, &foundUsage, &foundRegion, status); } U_ASSERT(foundCategory); U_ASSERT(foundUsage); if (!foundRegion) { - if (uprv_strcmp(region, "001") != 0) { - region = "001"; - idx = binarySearch(metadata, category, usage, region, &foundCategory, &foundUsage, - &foundRegion, status); + if (uprv_strcmp(desired.region.data(), "001") != 0) { + desired.region.truncate(0).append("001", status); + idx = binarySearch(metadata, desired, &foundCategory, &foundUsage, &foundRegion, status); } if (!foundRegion) { status = U_MISSING_RESOURCE_ERROR; - return idx; + return -1; } } U_ASSERT(foundCategory); diff --git a/icu4c/source/i18n/unitsdata.h b/icu4c/source/i18n/unitsdata.h index e0edf9ec493..061538e8bf4 100644 --- a/icu4c/source/i18n/unitsdata.h +++ b/icu4c/source/i18n/unitsdata.h @@ -106,12 +106,15 @@ namespace { /** * Metadata about the preferences in UnitPreferences::unitPrefs_. * + * This class owns all of its data. + * * UnitPreferenceMetadata lives in the anonymous namespace, because it should * only be useful to internal code and unit testing code. */ class U_I18N_API UnitPreferenceMetadata : public UMemory { public: UnitPreferenceMetadata(){}; + // Constructor, makes copies of the parameters passed to it. UnitPreferenceMetadata(const char *category, const char *usage, const char *region, int32_t prefsOffset, int32_t prefsCount, UErrorCode &status); @@ -139,9 +142,6 @@ class U_I18N_API UnitPreferenceMetadata : public UMemory { /** * Unit Preferences information for various locales and usages. - * - * TODO(hugovdm): add a function to look up the category based on the input - * unit. */ class U_I18N_API UnitPreferences { public: @@ -153,12 +153,11 @@ class U_I18N_API UnitPreferences { UnitPreferences(UErrorCode &status); /** - * Returns the set of unit preferences in the particular cateogry that best + * Returns the set of unit preferences in the particular category that best * matches the specified usage and region. * * If region can't be found, falls back to global (001). If usage can't be - * found, falls back to "default". Copies the preferences structures. - * TODO(hugovdm/review): Consider returning pointers (references) instead? + * found, falls back to "default". * * @param category The category within which to look up usage and region. * (TODO(hugovdm): improve docs on how to find the category, once the lookup @@ -170,10 +169,13 @@ class U_I18N_API UnitPreferences { * @param region The region whose preferences are desired. If there are no * specific preferences for the requested region, the method automatically * falls back to region "001" ("world"). - * @param outPreferences The vector to which preferences will be added. + * @param outPreferences A pointer into an array of preferences: essentially + * an array slice in combination with preferenceCount. + * @param preferenceCount The number of unit preferences that belong to the + * result set. * @param status Receives status. * - * TODO: maybe replace `UnitPreference **&outPrefrences` with a slice class? + * TODO(hugovdm): maybe replace `UnitPreference **&outPrefrences` with a slice class? */ void getPreferencesFor(const char *category, const char *usage, const char *region, const UnitPreference *const *&outPreferences, int32_t &preferenceCount, From 10c8ffe7ee27b0ea0a4109efd8e8bf653bc5f77b Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Thu, 28 May 2020 00:33:11 +0200 Subject: [PATCH 3/3] Add an error check, as per code review. --- 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 1052d1080f2..7533bb217b9 100644 --- a/icu4c/source/i18n/unitsdata.cpp +++ b/icu4c/source/i18n/unitsdata.cpp @@ -328,6 +328,7 @@ int32_t getPreferenceMetadataIndex(const MaybeStackVector