From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Tue, 14 Apr 2020 14:33:59 +0000 (+0200) Subject: Add getAllConversionRates(), drop selective code. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=2deeb28f3c51aa16e351e6c70ea547cb5cef6679;p=icu Add getAllConversionRates(), drop selective code. --- diff --git a/icu4c/source/i18n/unitsdata.cpp b/icu4c/source/i18n/unitsdata.cpp index 50142a6bf54..6bebed26400 100644 --- a/icu4c/source/i18n/unitsdata.cpp +++ b/icu4c/source/i18n/unitsdata.cpp @@ -11,6 +11,7 @@ #include "resource.h" #include "unitsdata.h" #include "uresimp.h" +#include "util.h" U_NAMESPACE_BEGIN @@ -53,116 +54,69 @@ class ConversionRateDataSink : public ResourceSink { */ 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; - - // Collect base unit, factor and offset from the resource. - int32_t lenSource = uprv_strlen(source); - const UChar *baseUnit = NULL, *factor = NULL, *offset = NULL; - int32_t lenBaseUnit, lenFactor, lenOffset; - const char *key; - for (int32_t i = 0; conversionRateTable.getKeyAndValue(i, key, value); ++i) { - if (uprv_strcmp(key, "target") == 0) { - baseUnit = value.getString(lenBaseUnit, status); - } else if (uprv_strcmp(key, "factor") == 0) { - factor = value.getString(lenFactor, status); - } else if (uprv_strcmp(key, "offset") == 0) { - 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; + if (uprv_strcmp(source, "convertUnits") != 0) { + status = U_ILLEGAL_ARGUMENT_ERROR; return; } - - // Check if we already have the conversion rate in question. - // - // 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 - // units.xml should all point at a defined base unit for the unit - // category. I should make this code more efficient after - // double-checking we're fine with relying on such a detail from the - // CLDR spec? - CharString lastBaseUnit; - lastBaseUnit.appendInvariantChars(baseUnit, lenBaseUnit, status); - if (U_FAILURE(status)) return; - for (int32_t i = 0, len = outVector.length(); i < len; i++) { - if (strcmp(outVector[i]->sourceUnit.data(), source) == 0 && - strcmp(outVector[i]->baseUnit.data(), lastBaseUnit.data()) == 0) { + ResourceTable conversionRateTable = value.getTable(status); + const char *srcUnit; + // We're reusing `value`, which seems to be a common pattern: + for (int32_t unit = 0; conversionRateTable.getKeyAndValue(unit, srcUnit, value); unit++) { + ResourceTable unitTable = value.getTable(status); + const char *key; + UnicodeString baseUnit = ICU_Utility::makeBogusString(); + UnicodeString factor = ICU_Utility::makeBogusString(); + UnicodeString offset = ICU_Utility::makeBogusString(); + for (int32_t i = 0; unitTable.getKeyAndValue(i, key, value); i++) { + if (uprv_strcmp(key, "target") == 0) { + baseUnit = value.getUnicodeString(status); + } else if (uprv_strcmp(key, "factor") == 0) { + factor = value.getUnicodeString(status); + } else if (uprv_strcmp(key, "offset") == 0) { + offset = value.getUnicodeString(status); + } + } + if (U_FAILURE(status)) return; + if (baseUnit.isBogus() || factor.isBogus()) { + // We could not find a usable conversion rate: bad resource. + status = U_MISSING_RESOURCE_ERROR; return; } - } - // We don't have this ConversionRateInfo yet: add it. - ConversionRateInfo *cr = outVector.emplaceBack(); - if (!cr) { - status = U_MEMORY_ALLOCATION_ERROR; - return; - } else { - cr->sourceUnit.append(source, lenSource, status); - cr->baseUnit.append(lastBaseUnit.data(), lastBaseUnit.length(), status); - cr->factor.appendInvariantChars(factor, lenFactor, status); - if (offset != NULL) cr->offset.appendInvariantChars(offset, lenOffset, status); + // We don't have this ConversionRateInfo yet: add it. + ConversionRateInfo *cr = outVector.emplaceBack(); + if (!cr) { + status = U_MEMORY_ALLOCATION_ERROR; + return; + } else { + cr->sourceUnit.append(srcUnit, status); + cr->baseUnit.appendInvariantChars(baseUnit, status); + cr->factor.appendInvariantChars(factor, status); + if (!offset.isBogus()) cr->offset.appendInvariantChars(offset, status); + } } + return; } private: MaybeStackVector &outVector; }; -/** - * Collects conversion information for a "SINGLE" unit (a unit whose complexity - * is UMEASURE_UNIT_SINGLE). For a COMPOUND or SEQUENCE unit an error will - * occur. - * - * @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 status The standard ICU error code output parameter. - */ -void processSingleUnit(const MeasureUnit &unit, const UResourceBundle *convertUnitsBundle, - ConversionRateDataSink &convertSink, 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); - } - ures_getAllItemsWithFallback(convertUnitsBundle, simple.getIdentifier(), convertSink, status); -} - } // namespace -MaybeStackVector U_I18N_API -getConversionRatesInfo(const MaybeStackVector &units, UErrorCode &status) { +MaybeStackVector U_I18N_API getAllConversionRates(UErrorCode &status) { MaybeStackVector result; - if (U_FAILURE(status)) return result; - LocalUResourceBundlePointer unitsBundle(ures_openDirect(NULL, "units", &status)); - StackUResourceBundle convertUnitsBundle; - ures_getByKey(unitsBundle.getAlias(), "convertUnits", convertUnitsBundle.getAlias(), &status); - ConversionRateDataSink convertSink(result); - - for (int i = 0; i < units.length(); i++) { - int32_t numSingleUnits; - LocalArray singleUnits = units[i]->splitToSingleUnits(numSingleUnits, status); - - for (int i = 0; i < numSingleUnits; i++) { - processSingleUnit(singleUnits[i], convertUnitsBundle.getAlias(), convertSink, status); - } - } + ConversionRateDataSink sink(result); + ures_getAllItemsWithFallback(unitsBundle.getAlias(), "convertUnits", sink, status); return result; } +MaybeStackVector +U_I18N_API getConversionRatesInfo(const MaybeStackVector &, UErrorCode &status) { + return getAllConversionRates(status); +} + U_NAMESPACE_END #endif /* #if !UCONFIG_NO_FORMATTING */ diff --git a/icu4c/source/i18n/unitsdata.h b/icu4c/source/i18n/unitsdata.h index 5f97143d1e0..87e49a8ec6f 100644 --- a/icu4c/source/i18n/unitsdata.h +++ b/icu4c/source/i18n/unitsdata.h @@ -37,6 +37,13 @@ class U_I18N_API ConversionRateInfo { CharString offset; }; +/** + * Returns ConversionRateInfo for all supported conversions. + * + * @param status Receives status. + */ +MaybeStackVector U_I18N_API getAllConversionRates(UErrorCode &status); + /** * Collects and returns ConversionRateInfo needed for conversions for a set of * units. diff --git a/icu4c/source/test/intltest/unitsdatatest.cpp b/icu4c/source/test/intltest/unitsdatatest.cpp index bab60f87956..7c42e2ff5c5 100644 --- a/icu4c/source/test/intltest/unitsdatatest.cpp +++ b/icu4c/source/test/intltest/unitsdatatest.cpp @@ -12,6 +12,7 @@ class UnitsDataTest : public IntlTest { void runIndexedTest(int32_t index, UBool exec, const char *&name, char *par = NULL); + void testGetAllConversionRates(); void testGetConversionRateInfo(); }; @@ -20,10 +21,28 @@ extern IntlTest *createUnitsDataTest() { return new UnitsDataTest(); } void UnitsDataTest::runIndexedTest(int32_t index, UBool exec, const char *&name, char * /*par*/) { if (exec) { logln("TestSuite UnitsDataTest: "); } TESTCASE_AUTO_BEGIN; + TESTCASE_AUTO(testGetAllConversionRates); TESTCASE_AUTO(testGetConversionRateInfo); TESTCASE_AUTO_END; } +void UnitsDataTest::testGetAllConversionRates() { + IcuTestErrorCode status(*this, "testGetAllConversionRates"); + MaybeStackVector conversionInfo = getAllConversionRates(status); + + // Convenience output for debugging + for (int i = 0; i < conversionInfo.length(); i++) { + ConversionRateInfo *cri = conversionInfo[i]; + logln("* conversionInfo %d: source=\"%s\", baseUnit=\"%s\", factor=\"%s\", offset=\"%s\"", i, + cri->sourceUnit.data(), cri->baseUnit.data(), cri->factor.data(), cri->offset.data()); + assertTrue("sourceUnit", cri->sourceUnit.length() > 0); + assertTrue("baseUnit", cri->baseUnit.length() > 0); + assertTrue("factor", cri->factor.length() > 0); + } +} + +// TODO(hugovdm): drop this test case, maybe even before ever merging it into +// units-staging. Not immediately deleting it to prove backward compatibility: void UnitsDataTest::testGetConversionRateInfo() { const int MAX_NUM_RATES = 5; struct { @@ -92,16 +111,18 @@ void UnitsDataTest::testGetConversionRateInfo() { } assertTrue(UnicodeString("<") + expected + "> expected", found); } - assertEquals("number of conversion rates", countExpected, conversionInfo.length()); - - // Convenience output for debugging - for (int i = 0; i < conversionInfo.length(); i++) { - ConversionRateInfo *cri = conversionInfo[i]; - logln("* conversionInfo %d: source=\"%s\", baseUnit=\"%s\", factor=\"%s\", " - "offset=\"%s\"", - i, cri->sourceUnit.data(), cri->baseUnit.data(), cri->factor.data(), - cri->offset.data()); - } + // We're now fetching all units, not just the needed ones, so this is no + // longer a valid test: + // assertEquals("number of conversion rates", countExpected, conversionInfo.length()); + + // // Convenience output for debugging + // for (int i = 0; i < conversionInfo.length(); i++) { + // ConversionRateInfo *cri = conversionInfo[i]; + // logln("* conversionInfo %d: source=\"%s\", baseUnit=\"%s\", factor=\"%s\", " + // "offset=\"%s\"", + // i, cri->sourceUnit.data(), cri->baseUnit.data(), cri->factor.data(), + // cri->offset.data()); + // } } }