From 04eac5b9adfe89904d6cd1c0678d64fdba5e1cf5 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Sat, 28 Mar 2020 19:47:32 +0100 Subject: [PATCH 01/22] Manual cherry-pick of getConversionRatesInfo --- icu4c/source/i18n/Makefile.in | 2 +- icu4c/source/i18n/i18n.vcxproj | 2 + icu4c/source/i18n/i18n.vcxproj.filters | 6 + icu4c/source/i18n/i18n_uwp.vcxproj | 2 + icu4c/source/i18n/unitsdata.cpp | 238 +++++++++++++++++++ icu4c/source/i18n/unitsdata.h | 60 +++++ icu4c/source/test/intltest/Makefile.in | 2 +- icu4c/source/test/intltest/itformat.cpp | 10 + icu4c/source/test/intltest/unitsdatatest.cpp | 123 ++++++++++ 9 files changed, 443 insertions(+), 2 deletions(-) create mode 100644 icu4c/source/i18n/unitsdata.cpp create mode 100644 icu4c/source/i18n/unitsdata.h create mode 100644 icu4c/source/test/intltest/unitsdatatest.cpp diff --git a/icu4c/source/i18n/Makefile.in b/icu4c/source/i18n/Makefile.in index 827f2c207ba..fe525c51012 100644 --- a/icu4c/source/i18n/Makefile.in +++ b/icu4c/source/i18n/Makefile.in @@ -115,7 +115,7 @@ numparse_affixes.o numparse_compositions.o numparse_validators.o \ numrange_fluent.o numrange_impl.o \ erarules.o \ formattedvalue.o formattedval_iterimpl.o formattedval_sbimpl.o formatted_string_builder.o \ -unitconverter.o +unitconverter.o unitsdata.o ## Header files to install HEADERS = $(srcdir)/unicode/*.h diff --git a/icu4c/source/i18n/i18n.vcxproj b/icu4c/source/i18n/i18n.vcxproj index f5cad11f522..8d8c2d265ae 100644 --- a/icu4c/source/i18n/i18n.vcxproj +++ b/icu4c/source/i18n/i18n.vcxproj @@ -267,6 +267,7 @@ + @@ -502,6 +503,7 @@ + diff --git a/icu4c/source/i18n/i18n.vcxproj.filters b/icu4c/source/i18n/i18n.vcxproj.filters index 5feb9f252fb..319648d785b 100644 --- a/icu4c/source/i18n/i18n.vcxproj.filters +++ b/icu4c/source/i18n/i18n.vcxproj.filters @@ -654,6 +654,9 @@ formatting + + formatting + @@ -1001,6 +1004,9 @@ formatting + + formatting + formatting diff --git a/icu4c/source/i18n/i18n_uwp.vcxproj b/icu4c/source/i18n/i18n_uwp.vcxproj index 39b80d454d4..94381983ab7 100644 --- a/icu4c/source/i18n/i18n_uwp.vcxproj +++ b/icu4c/source/i18n/i18n_uwp.vcxproj @@ -486,6 +486,7 @@ + @@ -721,6 +722,7 @@ + diff --git a/icu4c/source/i18n/unitsdata.cpp b/icu4c/source/i18n/unitsdata.cpp new file mode 100644 index 00000000000..b39005b5cb7 --- /dev/null +++ b/icu4c/source/i18n/unitsdata.cpp @@ -0,0 +1,238 @@ +// © 2020 and later: Unicode, Inc. and others. +// License & terms of use: http://www.unicode.org/copyright.html + +#include "unicode/utypes.h" + +#if !UCONFIG_NO_FORMATTING + +#include + +#include "cstring.h" +#include "resource.h" +#include "unitsdata.h" +#include "uresimp.h" + +U_NAMESPACE_BEGIN + +namespace { + +/** + * A ResourceSink that collects conversion rate information. + * + * This class is for use by ures_getAllItemsWithFallback. Example code for + * collecting conversion info for "mile" and "foot" into conversionInfoOutput: + * + * UErrorCode status = U_ZERO_ERROR; + * ures_getByKey(unitsBundle, "convertUnits", &fillIn, &status); + * MaybeStackVector conversionInfoOutput; + * ConversionRateDataSink convertSink(conversionInfoOutput); + * ures_getAllItemsWithFallback(fillIn, "mile", convertSink, status); + * ures_getAllItemsWithFallback(fillIn, "foot", convertSink, status); + */ +class ConversionRateDataSink : public ResourceSink { + public: + /** + * Constructor. + * @param out The vector to which ConversionRateInfo instances are to be + * added. + */ + explicit ConversionRateDataSink(MaybeStackVector &out) : outVector(out) {} + + /** + * Adds the conversion rate information found in value to the output vector. + * + * Each call to put() collects a ConversionRateInfo instance for the + * specified source unit identifier into the vector passed to the + * constructor, but only if an identical instance isn't already present. + * + * @param source The source unit identifier. + * @param value A resource containing conversion rate info (the base unit + * and factor, and possibly an offset). + * @param noFallback Ignored. + * @param status The standard ICU error code output parameter. + */ + void put(const char *source, ResourceValue &value, UBool /*noFallback*/, UErrorCode &status) { + 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 (baseUnit == NULL || factor == NULL) { + 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 + // 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? + fLastBaseUnit.clear(); + fLastBaseUnit.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(), fLastBaseUnit.data()) == 0) { + return; + } + } + if (U_FAILURE(status)) 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(fLastBaseUnit.data(), fLastBaseUnit.length(), status); + cr->factor.appendInvariantChars(factor, lenFactor, status); + if (offset != NULL) cr->offset.appendInvariantChars(offset, lenOffset, status); + } + } + + /** + * Returns the MeasureUnit that was the conversion base unit of the most + * recent call to put() - typically meaning the most recent call to + * ures_getAllItemsWithFallback(). + */ + MeasureUnit getLastBaseUnit(UErrorCode &status) { + return MeasureUnit::forIdentifier(fLastBaseUnit.data(), status); + } + + private: + MaybeStackVector &outVector; + + // TODO(review): felt like a hack: provides easy access to the most recent + // baseUnit. This hack is another point making me wonder if doing this + // ResourceSink thing is worthwhile. Functional style is not more verbose, + // and IMHO more readable than this object-based approach where the output + // seems/feels like a side-effect. + CharString fLastBaseUnit; +}; + +// The input unit needs to be simple, but can have dimensionality != 1. +void processSingleUnit(const MeasureUnit &unit, const UResourceBundle *convertUnitsBundle, + ConversionRateDataSink &convertSink, MeasureUnit *baseSingleUnit, + UErrorCode &status) { + int32_t dimensionality = unit.getDimensionality(status); + + 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); + + if (baseSingleUnit != NULL) { + MeasureUnit baseUnit = convertSink.getLastBaseUnit(status); + + 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: + 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; + } + } +} + +} // namespace + +MaybeStackVector getConversionRatesInfo(const MeasureUnit source, const MeasureUnit target, + MeasureUnit *baseCompoundUnit, + UErrorCode &status) { + MaybeStackVector result; + + int32_t sourceUnitsLength, targetUnitsLength; + LocalArray sourceUnits = source.splitToSingleUnits(sourceUnitsLength, status); + LocalArray targetUnits = target.splitToSingleUnits(targetUnitsLength, status); + + 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; + processSingleUnit(sourceUnits[i], convertUnitsBundle.getAlias(), convertSink, &baseUnit, status); + if (source.getComplexity(status) == UMEASURE_UNIT_SEQUENCE) { + if (i == 0) { + sourceBaseUnit = baseUnit; + } else { + if (baseUnit != sourceBaseUnit) { + status = U_ILLEGAL_ARGUMENT_ERROR; + return result; + } + } + } else { + sourceBaseUnit = sourceBaseUnit.product(baseUnit, status); + } + } + MeasureUnit targetBaseUnit; + for (int i = 0; i < targetUnitsLength; i++) { + MeasureUnit baseUnit; + processSingleUnit(targetUnits[i], convertUnitsBundle.getAlias(), convertSink, &baseUnit, status); + if (target.getComplexity(status) == UMEASURE_UNIT_SEQUENCE) { + // WIP/TODO(hugovdm): add consistency checks. + if (baseUnit != sourceBaseUnit) { + status = U_ILLEGAL_ARGUMENT_ERROR; + return result; + } + targetBaseUnit = baseUnit; + } else { + // WIP/FIXME(hugovdm): I think I found a bug in targetBaseUnit.product(): + // Target Base: x => + // + // fprintf(stderr, "Target Base: <%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)); + } + } + if (targetBaseUnit != sourceBaseUnit) { + status = U_ILLEGAL_ARGUMENT_ERROR; + return result; + } + if (baseCompoundUnit != NULL) { *baseCompoundUnit = sourceBaseUnit; } + return result; +} + +U_NAMESPACE_END + +#endif /* #if !UCONFIG_NO_FORMATTING */ diff --git a/icu4c/source/i18n/unitsdata.h b/icu4c/source/i18n/unitsdata.h new file mode 100644 index 00000000000..a3e106d0672 --- /dev/null +++ b/icu4c/source/i18n/unitsdata.h @@ -0,0 +1,60 @@ +// © 2020 and later: Unicode, Inc. and others. +// License & terms of use: http://www.unicode.org/copyright.html + +#include "unicode/utypes.h" + +#if !UCONFIG_NO_FORMATTING +#ifndef __GETUNITSDATA_H__ +#define __GETUNITSDATA_H__ + +#include "charstr.h" +#include "cmemory.h" +#include "unicode/measunit.h" +#include "unicode/stringpiece.h" +#include "unicode/utypes.h" + +U_NAMESPACE_BEGIN + +// Encapsulates "convertUnits" information from units resources, specifying how +// to convert from one unit to another. +class U_I18N_API ConversionRateInfo { + public: + ConversionRateInfo(){}; + ConversionRateInfo(StringPiece sourceUnit, StringPiece baseUnit, StringPiece factor, + StringPiece offset, UErrorCode &status) + : sourceUnit(), baseUnit(), factor(), offset() { + this->sourceUnit.append(sourceUnit, status); + this->baseUnit.append(baseUnit, status); + this->factor.append(factor, status); + this->offset.append(offset, status); + }; + CharString sourceUnit; + CharString baseUnit; // FIXME/WIP: baseUnit + CharString factor; + CharString offset; + bool reciprocal = false; +}; + +/** + * Collects and returns ConversionRateInfo needed to convert from source 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 status Receives status. + */ +MaybeStackVector U_I18N_API getConversionRatesInfo(MeasureUnit source, + MeasureUnit target, + MeasureUnit *baseCompoundUnit, + UErrorCode &status); + +U_NAMESPACE_END + +#endif //__GETUNITSDATA_H__ + +#endif /* #if !UCONFIG_NO_FORMATTING */ diff --git a/icu4c/source/test/intltest/Makefile.in b/icu4c/source/test/intltest/Makefile.in index 594d491f6f7..74a6f907564 100644 --- a/icu4c/source/test/intltest/Makefile.in +++ b/icu4c/source/test/intltest/Makefile.in @@ -69,7 +69,7 @@ string_segment_test.o \ numbertest_parse.o numbertest_doubleconversion.o numbertest_skeletons.o \ static_unisets_test.o numfmtdatadriventest.o numbertest_range.o erarulestest.o \ formattedvaluetest.o formatted_string_builder_test.o numbertest_permutation.o \ -unitstest.o +unitsdatatest.o unitstest.o DEPS = $(OBJECTS:.o=.d) diff --git a/icu4c/source/test/intltest/itformat.cpp b/icu4c/source/test/intltest/itformat.cpp index 9126d528fa2..bd804888fbf 100644 --- a/icu4c/source/test/intltest/itformat.cpp +++ b/icu4c/source/test/intltest/itformat.cpp @@ -74,6 +74,7 @@ extern IntlTest *createScientificNumberFormatterTest(); extern IntlTest *createFormattedValueTest(); extern IntlTest *createFormattedStringBuilderTest(); extern IntlTest *createStringSegmentTest(); +extern IntlTest *createUnitsDataTest(); extern IntlTest *createUnitsTest(); @@ -257,6 +258,15 @@ void IntlTestFormat::runIndexedTest( int32_t index, UBool exec, const char* &nam callTest(*test, par); } break; + case 57: + name = "UnitsDataTest"; + if (exec) { + logln("UnitsDataTest test---"); + logln((UnicodeString)""); + LocalPointer test(createUnitsDataTest()); + callTest(*test, par); + } + break; default: name = ""; break; //needed to end loop } if (exec) { diff --git a/icu4c/source/test/intltest/unitsdatatest.cpp b/icu4c/source/test/intltest/unitsdatatest.cpp new file mode 100644 index 00000000000..225482d80c8 --- /dev/null +++ b/icu4c/source/test/intltest/unitsdatatest.cpp @@ -0,0 +1,123 @@ +// © 2020 and later: Unicode, Inc. and others. +// License & terms of use: http://www.unicode.org/copyright.html#License + +#if !UCONFIG_NO_FORMATTING + +#include "intltest.h" +#include "unitsdata.h" + +class UnitsDataTest : public IntlTest { + public: + UnitsDataTest() {} + + void runIndexedTest(int32_t index, UBool exec, const char *&name, char *par = NULL); + + void testGetConversionRateInfo(); +}; + +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(testGetConversionRateInfo); + TESTCASE_AUTO_END; +} + +void UnitsDataTest::testGetConversionRateInfo() { + const int MAX_NUM_RATES = 5; + struct { + // The source unit passed to getConversionRateInfo. + const char *sourceUnit; + // The target unit passed to getConversionRateInfo. + const char *targetUnit; + // Expected: units whose conversion rates are expected in the results. + const char *expectedOutputs[MAX_NUM_RATES]; + // Expected "base unit", to serve as pivot between source and target. + const char *expectedBaseUnit; + } testCases[]{ + {"centimeter-per-square-milligram", + "inch-per-square-ounce", + {"meter", "gram", "inch", "ounce", NULL}, + "meter-per-square-kilogram"}, + + {"liter", "gallon", {"liter", "gallon", NULL, NULL, NULL}, "cubic-meter"}, + + // Sequence + {"stone-and-pound", "ton", {"pound", "stone", "ton", NULL, NULL}, "kilogram"}, + + {"mile-per-hour", + "dekameter-per-hour", + {"mile", "hour", "meter", NULL, NULL}, + "meter-per-second"}, + + // Power: watt + {"watt", + "horsepower", + {"watt", "horsepower", NULL, NULL, NULL}, + "kilogram-square-meter-per-cubic-second"}, + + // Energy: joule + {"therm-us", + "kilogram-square-meter-per-square-second", + {"therm-us", "kilogram", "meter", "second", NULL}, + "kilogram-square-meter-per-square-second"}, + + // WIP/FIXME(hugovdm): I think I found a bug in targetBaseUnit.product(): + // Target Base: x => + // + // // Joule-per-meter + // {"therm-us-per-meter", + // "joule-per-meter", + // {"therm-us", "joule", "meter", NULL, NULL}, + // "kilogram-meter-per-square-second"}, + + // TODO: include capacitance test case with base unit: + // pow4-second-square-ampere-per-kilogram-square-meter; + }; + for (const auto &t : testCases) { + logln("---testing: source=\"%s\", target=\"%s\", expectedBaseUnit=\"%s\"", t.sourceUnit, + t.targetUnit, t.expectedBaseUnit); + IcuTestErrorCode status(*this, "testGetConversionRateInfo"); + + MeasureUnit baseCompoundUnit; + MeasureUnit sourceUnit = MeasureUnit::forIdentifier(t.sourceUnit, status); + MeasureUnit targetUnit = MeasureUnit::forIdentifier(t.targetUnit, status); + MaybeStackVector conversionInfo = + getConversionRatesInfo(sourceUnit, targetUnit, &baseCompoundUnit, status); + if (status.errIfFailureAndReset("getConversionRatesInfo(<%s>, <%s>, ...)", + sourceUnit.getIdentifier(), targetUnit.getIdentifier())) { + continue; + } + + assertEquals("baseCompoundUnit returned by getConversionRatesInfo", t.expectedBaseUnit, + baseCompoundUnit.getIdentifier()); + int countExpected; + for (countExpected = 0; countExpected < MAX_NUM_RATES; countExpected++) { + auto expected = t.expectedOutputs[countExpected]; + if (expected == NULL) break; + // Check if this conversion rate was expected + bool found = false; + for (int i = 0; i < conversionInfo.length(); i++) { + auto cri = conversionInfo[i]; + if (strcmp(expected, cri->sourceUnit.data()) == 0) { + found = true; + break; + } + } + 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()); + } + } +} + +#endif /* #if !UCONFIG_NO_FORMATTING */ From e11de4b17b9ababb188c323a0ec77b65bdcbf833 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Sat, 28 Mar 2020 20:56:51 +0100 Subject: [PATCH 02/22] Clean up getConversionRatesInfo to prepare for review. --- icu4c/source/i18n/unitsdata.cpp | 79 +++++++++++++++++++++++---------- icu4c/source/i18n/unitsdata.h | 15 ++++--- 2 files changed, 64 insertions(+), 30 deletions(-) diff --git a/icu4c/source/i18n/unitsdata.cpp b/icu4c/source/i18n/unitsdata.cpp index b39005b5cb7..8159e4d45dc 100644 --- a/icu4c/source/i18n/unitsdata.cpp +++ b/icu4c/source/i18n/unitsdata.cpp @@ -52,6 +52,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; @@ -69,14 +70,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 @@ -93,7 +96,6 @@ class ConversionRateDataSink : public ResourceSink { return; } } - if (U_FAILURE(status)) return; // We don't have this ConversionRateInfo yet: add it. ConversionRateInfo *cr = outVector.emplaceBack(); @@ -128,12 +130,39 @@ class ConversionRateDataSink : public ResourceSink { CharString fLastBaseUnit; }; -// 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); @@ -146,24 +175,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; } @@ -172,10 +191,12 @@ void processSingleUnit(const MeasureUnit &unit, const UResourceBundle *convertUn } // namespace -MaybeStackVector getConversionRatesInfo(const MeasureUnit source, const MeasureUnit target, - MeasureUnit *baseCompoundUnit, +MaybeStackVector getConversionRatesInfo(const MeasureUnit source, + const MeasureUnit target, + MeasureUnit *baseUnit, UErrorCode &status) { MaybeStackVector result; + if (U_FAILURE(status)) return result; int32_t sourceUnitsLength, targetUnitsLength; LocalArray sourceUnits = source.splitToSingleUnits(sourceUnitsLength, status); @@ -184,8 +205,8 @@ MaybeStackVector 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; @@ -203,6 +224,7 @@ MaybeStackVector getConversionRatesInfo(const MeasureUnit so sourceBaseUnit = sourceBaseUnit.product(baseUnit, status); } } + MeasureUnit targetBaseUnit; for (int i = 0; i < targetUnitsLength; i++) { MeasureUnit baseUnit; @@ -215,21 +237,30 @@ MaybeStackVector getConversionRatesInfo(const MeasureUnit so } targetBaseUnit = baseUnit; } else { - // WIP/FIXME(hugovdm): I think I found a bug in targetBaseUnit.product(): - // Target Base: x => + // 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. // - // fprintf(stderr, "Target Base: <%s> x <%s> => ", targetBaseUnit.getIdentifier(), + // The product was: + // + // * => + // + // as output by a printf: + // + // 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 a3e106d0672..9893c23f09e 100644 --- a/icu4c/source/i18n/unitsdata.h +++ b/icu4c/source/i18n/unitsdata.h @@ -17,6 +17,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(){}; @@ -29,28 +32,28 @@ 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; }; /** * 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 U_I18N_API getConversionRatesInfo(MeasureUnit source, MeasureUnit target, - MeasureUnit *baseCompoundUnit, + MeasureUnit *baseUnit, UErrorCode &status); U_NAMESPACE_END From a1f6fa319f824412eaae4313395a3a4040c1e6ba Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Sat, 28 Mar 2020 21:10:02 +0100 Subject: [PATCH 03/22] clang-format pass over new files, for good measure --- icu4c/source/i18n/unitsdata.cpp | 3 +-- icu4c/source/test/intltest/unitsdatatest.cpp | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/icu4c/source/i18n/unitsdata.cpp b/icu4c/source/i18n/unitsdata.cpp index 8159e4d45dc..60ca54c5c42 100644 --- a/icu4c/source/i18n/unitsdata.cpp +++ b/icu4c/source/i18n/unitsdata.cpp @@ -193,8 +193,7 @@ void processSingleUnit(const MeasureUnit &unit, const UResourceBundle *convertUn MaybeStackVector getConversionRatesInfo(const MeasureUnit source, const MeasureUnit target, - MeasureUnit *baseUnit, - UErrorCode &status) { + MeasureUnit *baseUnit, UErrorCode &status) { MaybeStackVector result; if (U_FAILURE(status)) return result; diff --git a/icu4c/source/test/intltest/unitsdatatest.cpp b/icu4c/source/test/intltest/unitsdatatest.cpp index 225482d80c8..e5c51016009 100644 --- a/icu4c/source/test/intltest/unitsdatatest.cpp +++ b/icu4c/source/test/intltest/unitsdatatest.cpp @@ -3,11 +3,11 @@ #if !UCONFIG_NO_FORMATTING -#include "intltest.h" #include "unitsdata.h" +#include "intltest.h" class UnitsDataTest : public IntlTest { - public: + public: UnitsDataTest() {} void runIndexedTest(int32_t index, UBool exec, const char *&name, char *par = NULL); From da70f7632be932c07657a5c283c33ba80fa971c2 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Mon, 30 Mar 2020 14:14:02 +0200 Subject: [PATCH 04/22] Fix comments: remove a WIP, fix expectations in disabled/failing test. --- icu4c/source/i18n/unitsdata.cpp | 3 ++- icu4c/source/test/intltest/unitsdatatest.cpp | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/icu4c/source/i18n/unitsdata.cpp b/icu4c/source/i18n/unitsdata.cpp index 60ca54c5c42..6651a905768 100644 --- a/icu4c/source/i18n/unitsdata.cpp +++ b/icu4c/source/i18n/unitsdata.cpp @@ -229,7 +229,6 @@ MaybeStackVector getConversionRatesInfo(const MeasureUnit so MeasureUnit baseUnit; processSingleUnit(targetUnits[i], convertUnitsBundle.getAlias(), convertSink, &baseUnit, status); if (target.getComplexity(status) == UMEASURE_UNIT_SEQUENCE) { - // WIP/TODO(hugovdm): add consistency checks. if (baseUnit != sourceBaseUnit) { status = U_ILLEGAL_ARGUMENT_ERROR; return result; @@ -250,6 +249,8 @@ MaybeStackVector getConversionRatesInfo(const MeasureUnit so // fprintf(stderr, "<%s> x <%s> => ", // targetBaseUnit.getIdentifier(), // baseUnit.getIdentifier()); + // + // Expected: targetBaseUnit = targetBaseUnit.product(baseUnit, status); // fprintf(stderr, "<%s> - Status: %s\n", // targetBaseUnit.getIdentifier(), u_errorName(status)); diff --git a/icu4c/source/test/intltest/unitsdatatest.cpp b/icu4c/source/test/intltest/unitsdatatest.cpp index e5c51016009..32fa616f8fd 100644 --- a/icu4c/source/test/intltest/unitsdatatest.cpp +++ b/icu4c/source/test/intltest/unitsdatatest.cpp @@ -70,7 +70,7 @@ void UnitsDataTest::testGetConversionRateInfo() { // {"therm-us-per-meter", // "joule-per-meter", // {"therm-us", "joule", "meter", NULL, NULL}, - // "kilogram-meter-per-square-second"}, + // "kilogram-square-meter-per-square-second"}, // TODO: include capacitance test case with base unit: // pow4-second-square-ampere-per-kilogram-square-meter; From 07f594fd1464ea654bb2e0f2736066ae955804d8 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Mon, 30 Mar 2020 14:49:03 +0200 Subject: [PATCH 05/22] Fix mistake in commented-out identifier in previous commit. --- icu4c/source/test/intltest/unitsdatatest.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/icu4c/source/test/intltest/unitsdatatest.cpp b/icu4c/source/test/intltest/unitsdatatest.cpp index 32fa616f8fd..797480c6019 100644 --- a/icu4c/source/test/intltest/unitsdatatest.cpp +++ b/icu4c/source/test/intltest/unitsdatatest.cpp @@ -70,7 +70,7 @@ void UnitsDataTest::testGetConversionRateInfo() { // {"therm-us-per-meter", // "joule-per-meter", // {"therm-us", "joule", "meter", NULL, NULL}, - // "kilogram-square-meter-per-square-second"}, + // "kilogram-square-meter-per-meter-square-second"}, // TODO: include capacitance test case with base unit: // pow4-second-square-ampere-per-kilogram-square-meter; From aeacf029c833f42f80d23b93829a36484441ff48 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Mon, 30 Mar 2020 17:35:48 +0200 Subject: [PATCH 06/22] Add failing reciprocal test that we want to have passing. --- icu4c/source/test/intltest/unitsdatatest.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/icu4c/source/test/intltest/unitsdatatest.cpp b/icu4c/source/test/intltest/unitsdatatest.cpp index 797480c6019..3ca72db4e8c 100644 --- a/icu4c/source/test/intltest/unitsdatatest.cpp +++ b/icu4c/source/test/intltest/unitsdatatest.cpp @@ -63,6 +63,15 @@ void UnitsDataTest::testGetConversionRateInfo() { {"therm-us", "kilogram", "meter", "second", NULL}, "kilogram-square-meter-per-square-second"}, + // Add "reciprocal" example: consumption and consumption-inverse + // + // WIP/FIXME: failing example which should pass. Thus we will remove the + // base unit calculation. + {"liter-per-100-kilometer", + "mile-per-gallon", + {"meter", "mile", "gallon", NULL, NULL}, + "cubic-meter-per-meter"}, + // WIP/FIXME(hugovdm): I think I found a bug in targetBaseUnit.product(): // Target Base: x => // From cd32130c7f49b28271e314848161d8b3717c51b3 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Mon, 30 Mar 2020 18:05:29 +0200 Subject: [PATCH 07/22] Rip out compound base unit calculation. --- icu4c/source/i18n/unitsdata.cpp | 82 ++------------------ icu4c/source/i18n/unitsdata.h | 8 +- icu4c/source/test/intltest/unitsdatatest.cpp | 50 +++--------- 3 files changed, 16 insertions(+), 124 deletions(-) diff --git a/icu4c/source/i18n/unitsdata.cpp b/icu4c/source/i18n/unitsdata.cpp index 6651a905768..1fe90760015 100644 --- a/icu4c/source/i18n/unitsdata.cpp +++ b/icu4c/source/i18n/unitsdata.cpp @@ -151,14 +151,10 @@ class ConversionRateDataSink : public ResourceSink { * 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) { + ConversionRateDataSink &convertSink, UErrorCode &status) { if (U_FAILURE(status)) return; int32_t dimensionality = unit.getDimensionality(status); @@ -168,32 +164,12 @@ void processSingleUnit(const MeasureUnit &unit, const UResourceBundle *convertUn simple = unit.withDimensionality(1, status).withSIPrefix(UMEASURE_SI_PREFIX_ONE, status); } ures_getAllItemsWithFallback(convertUnitsBundle, simple.getIdentifier(), convertSink, status); - - if (baseSingleUnit != NULL) { - MeasureUnit baseUnit = convertSink.getLastBaseUnit(status); - - if (dimensionality == 1) { - *baseSingleUnit = baseUnit; - } else if (baseUnit.getComplexity(status) == UMEASURE_UNIT_SINGLE) { - // 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. - status = U_ILLEGAL_ARGUMENT_ERROR; - return; - } - } } } // namespace -MaybeStackVector getConversionRatesInfo(const MeasureUnit source, - const MeasureUnit target, - MeasureUnit *baseUnit, UErrorCode &status) { +MaybeStackVector +getConversionRatesInfo(const MeasureUnit source, const MeasureUnit target, UErrorCode &status) { MaybeStackVector result; if (U_FAILURE(status)) return result; @@ -206,61 +182,13 @@ MaybeStackVector getConversionRatesInfo(const MeasureUnit so ures_getByKey(unitsBundle.getAlias(), "convertUnits", convertUnitsBundle.getAlias(), &status); ConversionRateDataSink convertSink(result); - MeasureUnit sourceBaseUnit; for (int i = 0; i < sourceUnitsLength; i++) { - MeasureUnit baseUnit; - processSingleUnit(sourceUnits[i], convertUnitsBundle.getAlias(), convertSink, &baseUnit, status); - if (source.getComplexity(status) == UMEASURE_UNIT_SEQUENCE) { - if (i == 0) { - sourceBaseUnit = baseUnit; - } else { - if (baseUnit != sourceBaseUnit) { - status = U_ILLEGAL_ARGUMENT_ERROR; - return result; - } - } - } else { - sourceBaseUnit = sourceBaseUnit.product(baseUnit, status); - } + processSingleUnit(sourceUnits[i], convertUnitsBundle.getAlias(), convertSink, status); } - MeasureUnit targetBaseUnit; for (int i = 0; i < targetUnitsLength; i++) { - MeasureUnit baseUnit; - processSingleUnit(targetUnits[i], convertUnitsBundle.getAlias(), convertSink, &baseUnit, status); - if (target.getComplexity(status) == UMEASURE_UNIT_SEQUENCE) { - if (baseUnit != sourceBaseUnit) { - status = U_ILLEGAL_ARGUMENT_ERROR; - return result; - } - targetBaseUnit = baseUnit; - } else { - // 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: - // - // * => - // - // as output by a printf: - // - // fprintf(stderr, "<%s> x <%s> => ", - // targetBaseUnit.getIdentifier(), - // baseUnit.getIdentifier()); - // - // Expected: - targetBaseUnit = targetBaseUnit.product(baseUnit, status); - // fprintf(stderr, "<%s> - Status: %s\n", - // targetBaseUnit.getIdentifier(), u_errorName(status)); - } + processSingleUnit(targetUnits[i], convertUnitsBundle.getAlias(), convertSink, status); } - if (targetBaseUnit != sourceBaseUnit) { - status = U_ILLEGAL_ARGUMENT_ERROR; - return result; - } - if (baseUnit != NULL) { *baseUnit = sourceBaseUnit; } return result; } diff --git a/icu4c/source/i18n/unitsdata.h b/icu4c/source/i18n/unitsdata.h index 9893c23f09e..58aa989cdf5 100644 --- a/icu4c/source/i18n/unitsdata.h +++ b/icu4c/source/i18n/unitsdata.h @@ -38,22 +38,18 @@ class U_I18N_API ConversionRateInfo { }; /** - * Collects and returns ConversionRateInfo needed to convert from source to - * baseUnit and from target to baseUnit. + * Collects and returns ConversionRateInfo needed for conversions from source + * and to target. * * 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 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 U_I18N_API getConversionRatesInfo(MeasureUnit source, MeasureUnit target, - MeasureUnit *baseUnit, UErrorCode &status); U_NAMESPACE_END diff --git a/icu4c/source/test/intltest/unitsdatatest.cpp b/icu4c/source/test/intltest/unitsdatatest.cpp index 3ca72db4e8c..cd3e213fd76 100644 --- a/icu4c/source/test/intltest/unitsdatatest.cpp +++ b/icu4c/source/test/intltest/unitsdatatest.cpp @@ -33,74 +33,42 @@ void UnitsDataTest::testGetConversionRateInfo() { const char *targetUnit; // Expected: units whose conversion rates are expected in the results. const char *expectedOutputs[MAX_NUM_RATES]; - // Expected "base unit", to serve as pivot between source and target. - const char *expectedBaseUnit; } testCases[]{ {"centimeter-per-square-milligram", "inch-per-square-ounce", - {"meter", "gram", "inch", "ounce", NULL}, - "meter-per-square-kilogram"}, + {"meter", "gram", "inch", "ounce", NULL}}, - {"liter", "gallon", {"liter", "gallon", NULL, NULL, NULL}, "cubic-meter"}, + {"liter", "gallon", {"liter", "gallon", NULL, NULL, NULL}}, // Sequence - {"stone-and-pound", "ton", {"pound", "stone", "ton", NULL, NULL}, "kilogram"}, + {"stone-and-pound", "ton", {"pound", "stone", "ton", NULL, NULL}}, - {"mile-per-hour", - "dekameter-per-hour", - {"mile", "hour", "meter", NULL, NULL}, - "meter-per-second"}, + {"mile-per-hour", "dekameter-per-hour", {"mile", "hour", "meter", NULL, NULL}}, // Power: watt - {"watt", - "horsepower", - {"watt", "horsepower", NULL, NULL, NULL}, - "kilogram-square-meter-per-cubic-second"}, + {"watt", "horsepower", {"watt", "horsepower", NULL, NULL, NULL}}, // Energy: joule {"therm-us", "kilogram-square-meter-per-square-second", - {"therm-us", "kilogram", "meter", "second", NULL}, - "kilogram-square-meter-per-square-second"}, + {"therm-us", "kilogram", "meter", "second", NULL}}, // Add "reciprocal" example: consumption and consumption-inverse - // - // WIP/FIXME: failing example which should pass. Thus we will remove the - // base unit calculation. - {"liter-per-100-kilometer", - "mile-per-gallon", - {"meter", "mile", "gallon", NULL, NULL}, - "cubic-meter-per-meter"}, - - // WIP/FIXME(hugovdm): I think I found a bug in targetBaseUnit.product(): - // Target Base: x => - // - // // Joule-per-meter - // {"therm-us-per-meter", - // "joule-per-meter", - // {"therm-us", "joule", "meter", NULL, NULL}, - // "kilogram-square-meter-per-meter-square-second"}, - - // TODO: include capacitance test case with base unit: - // pow4-second-square-ampere-per-kilogram-square-meter; + {"liter-per-100-kilometer", "mile-per-gallon", {"liter", "100-kilometer", "mile", "gallon", NULL}}, }; for (const auto &t : testCases) { - logln("---testing: source=\"%s\", target=\"%s\", expectedBaseUnit=\"%s\"", t.sourceUnit, - t.targetUnit, t.expectedBaseUnit); + logln("---testing: source=\"%s\", target=\"%s\"", t.sourceUnit, t.targetUnit); IcuTestErrorCode status(*this, "testGetConversionRateInfo"); - MeasureUnit baseCompoundUnit; MeasureUnit sourceUnit = MeasureUnit::forIdentifier(t.sourceUnit, status); MeasureUnit targetUnit = MeasureUnit::forIdentifier(t.targetUnit, status); MaybeStackVector conversionInfo = - getConversionRatesInfo(sourceUnit, targetUnit, &baseCompoundUnit, status); + getConversionRatesInfo(sourceUnit, targetUnit, status); if (status.errIfFailureAndReset("getConversionRatesInfo(<%s>, <%s>, ...)", sourceUnit.getIdentifier(), targetUnit.getIdentifier())) { continue; } - assertEquals("baseCompoundUnit returned by getConversionRatesInfo", t.expectedBaseUnit, - baseCompoundUnit.getIdentifier()); int countExpected; for (countExpected = 0; countExpected < MAX_NUM_RATES; countExpected++) { auto expected = t.expectedOutputs[countExpected]; From 60285a7b677391c44b6a8f7c3f4c3a4b9fa5967f Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Mon, 30 Mar 2020 18:29:39 +0200 Subject: [PATCH 08/22] Reorder unit tests, drop unneeded comments. This code and these tests have become quite a bit simpler since we're no longer trying to calculate the 'compound base unit' here. --- icu4c/source/i18n/unitsdata.h | 3 --- icu4c/source/test/intltest/unitsdatatest.cpp | 23 ++++++++++---------- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/icu4c/source/i18n/unitsdata.h b/icu4c/source/i18n/unitsdata.h index 58aa989cdf5..a34d5557f50 100644 --- a/icu4c/source/i18n/unitsdata.h +++ b/icu4c/source/i18n/unitsdata.h @@ -41,9 +41,6 @@ class U_I18N_API ConversionRateInfo { * Collects and returns ConversionRateInfo needed for conversions from source * and to target. * - * 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 status Receives status. diff --git a/icu4c/source/test/intltest/unitsdatatest.cpp b/icu4c/source/test/intltest/unitsdatatest.cpp index cd3e213fd76..f61f85fe822 100644 --- a/icu4c/source/test/intltest/unitsdatatest.cpp +++ b/icu4c/source/test/intltest/unitsdatatest.cpp @@ -34,27 +34,26 @@ void UnitsDataTest::testGetConversionRateInfo() { // Expected: units whose conversion rates are expected in the results. const char *expectedOutputs[MAX_NUM_RATES]; } testCases[]{ - {"centimeter-per-square-milligram", - "inch-per-square-ounce", - {"meter", "gram", "inch", "ounce", NULL}}, - + {"meter", "kilometer", {"meter", NULL, NULL, NULL, NULL}}, {"liter", "gallon", {"liter", "gallon", NULL, NULL, NULL}}, + {"watt", "horsepower", {"watt", "horsepower", NULL, NULL, NULL}}, + {"mile-per-hour", "dekameter-per-hour", {"mile", "hour", "meter", NULL, NULL}}, // Sequence {"stone-and-pound", "ton", {"pound", "stone", "ton", NULL, NULL}}, - {"mile-per-hour", "dekameter-per-hour", {"mile", "hour", "meter", NULL, NULL}}, - - // Power: watt - {"watt", "horsepower", {"watt", "horsepower", NULL, NULL, NULL}}, - - // Energy: joule + // Compound + {"centimeter-per-square-milligram", + "inch-per-square-ounce", + {"meter", "gram", "inch", "ounce", NULL}}, {"therm-us", "kilogram-square-meter-per-square-second", {"therm-us", "kilogram", "meter", "second", NULL}}, - // Add "reciprocal" example: consumption and consumption-inverse - {"liter-per-100-kilometer", "mile-per-gallon", {"liter", "100-kilometer", "mile", "gallon", NULL}}, + // "Reciprocal" example: consumption and consumption-inverse + {"liter-per-100-kilometer", + "mile-per-gallon", + {"liter", "100-kilometer", "mile", "gallon", NULL}}, }; for (const auto &t : testCases) { logln("---testing: source=\"%s\", target=\"%s\"", t.sourceUnit, t.targetUnit); From 165069a597ef4afc58c10355d9992c25c07d5bfb Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Mon, 30 Mar 2020 20:47:40 +0200 Subject: [PATCH 09/22] Have getConversionRatesInfo accept a MaybeStackVector instead of only source and target. --- icu4c/source/i18n/unitsdata.cpp | 19 ++++++++---------- icu4c/source/i18n/unitsdata.h | 12 +++++------ icu4c/source/test/intltest/unitsdatatest.cpp | 21 ++++++++++++++------ 3 files changed, 28 insertions(+), 24 deletions(-) diff --git a/icu4c/source/i18n/unitsdata.cpp b/icu4c/source/i18n/unitsdata.cpp index 1fe90760015..2464ccbefa8 100644 --- a/icu4c/source/i18n/unitsdata.cpp +++ b/icu4c/source/i18n/unitsdata.cpp @@ -168,26 +168,23 @@ void processSingleUnit(const MeasureUnit &unit, const UResourceBundle *convertUn } // namespace -MaybeStackVector -getConversionRatesInfo(const MeasureUnit source, const MeasureUnit target, UErrorCode &status) { +MaybeStackVector U_I18N_API +getConversionRatesInfo(const MaybeStackVector &units, UErrorCode &status) { MaybeStackVector result; if (U_FAILURE(status)) return result; - int32_t sourceUnitsLength, targetUnitsLength; - LocalArray sourceUnits = source.splitToSingleUnits(sourceUnitsLength, status); - LocalArray targetUnits = target.splitToSingleUnits(targetUnitsLength, status); - 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 < sourceUnitsLength; i++) { - processSingleUnit(sourceUnits[i], convertUnitsBundle.getAlias(), convertSink, status); - } + for (int i = 0; i < units.length(); i++) { + int32_t numSingleUnits; + LocalArray singleUnits = units[i]->splitToSingleUnits(numSingleUnits, status); - for (int i = 0; i < targetUnitsLength; i++) { - processSingleUnit(targetUnits[i], convertUnitsBundle.getAlias(), convertSink, status); + for (int i = 0; i < numSingleUnits; i++) { + processSingleUnit(singleUnits[i], convertUnitsBundle.getAlias(), convertSink, status); + } } return result; } diff --git a/icu4c/source/i18n/unitsdata.h b/icu4c/source/i18n/unitsdata.h index a34d5557f50..5f97143d1e0 100644 --- a/icu4c/source/i18n/unitsdata.h +++ b/icu4c/source/i18n/unitsdata.h @@ -38,16 +38,14 @@ class U_I18N_API ConversionRateInfo { }; /** - * Collects and returns ConversionRateInfo needed for conversions from source - * and to target. + * Collects and returns ConversionRateInfo needed for conversions for a set of + * units. * - * @param source The source unit (the unit type converted from). - * @param target The target unit (the unit type converted to). + * @param units The units for which to load conversion data. * @param status Receives status. */ -MaybeStackVector U_I18N_API getConversionRatesInfo(MeasureUnit source, - MeasureUnit target, - UErrorCode &status); +MaybeStackVector + U_I18N_API getConversionRatesInfo(const MaybeStackVector &units, UErrorCode &status); U_NAMESPACE_END diff --git a/icu4c/source/test/intltest/unitsdatatest.cpp b/icu4c/source/test/intltest/unitsdatatest.cpp index f61f85fe822..bab60f87956 100644 --- a/icu4c/source/test/intltest/unitsdatatest.cpp +++ b/icu4c/source/test/intltest/unitsdatatest.cpp @@ -59,12 +59,21 @@ void UnitsDataTest::testGetConversionRateInfo() { logln("---testing: source=\"%s\", target=\"%s\"", t.sourceUnit, t.targetUnit); IcuTestErrorCode status(*this, "testGetConversionRateInfo"); - MeasureUnit sourceUnit = MeasureUnit::forIdentifier(t.sourceUnit, status); - MeasureUnit targetUnit = MeasureUnit::forIdentifier(t.targetUnit, status); - MaybeStackVector conversionInfo = - getConversionRatesInfo(sourceUnit, targetUnit, status); - if (status.errIfFailureAndReset("getConversionRatesInfo(<%s>, <%s>, ...)", - sourceUnit.getIdentifier(), targetUnit.getIdentifier())) { + MaybeStackVector units; + MeasureUnit *item = units.emplaceBack(MeasureUnit::forIdentifier(t.sourceUnit, status)); + if (!item) { + status.set(U_MEMORY_ALLOCATION_ERROR); + return; + } + item = units.emplaceBack(MeasureUnit::forIdentifier(t.targetUnit, status)); + if (!item) { + status.set(U_MEMORY_ALLOCATION_ERROR); + return; + } + + MaybeStackVector conversionInfo = getConversionRatesInfo(units, status); + if (status.errIfFailureAndReset("getConversionRatesInfo({<%s>, <%s>}, ...)", t.sourceUnit, + t.targetUnit)) { continue; } From faae51add6cae437bdf4f866a082604604b8c407 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Fri, 3 Apr 2020 11:52:41 +0200 Subject: [PATCH 10/22] fixup! Rip out compound base unit calculation. --- icu4c/source/i18n/unitsdata.cpp | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/icu4c/source/i18n/unitsdata.cpp b/icu4c/source/i18n/unitsdata.cpp index 2464ccbefa8..b80b50273a6 100644 --- a/icu4c/source/i18n/unitsdata.cpp +++ b/icu4c/source/i18n/unitsdata.cpp @@ -131,19 +131,9 @@ class ConversionRateDataSink : public ResourceSink { }; /** - * 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. + * 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. From 05a4a072516333a3aea0412cedf52618e47393dc Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Tue, 7 Apr 2020 16:14:24 +0200 Subject: [PATCH 11/22] fixup! Rip out compound base unit calculation. --- icu4c/source/i18n/unitsdata.cpp | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/icu4c/source/i18n/unitsdata.cpp b/icu4c/source/i18n/unitsdata.cpp index b80b50273a6..50142a6bf54 100644 --- a/icu4c/source/i18n/unitsdata.cpp +++ b/icu4c/source/i18n/unitsdata.cpp @@ -87,12 +87,12 @@ class ConversionRateDataSink : public ResourceSink { // category. I should make this code more efficient after // double-checking we're fine with relying on such a detail from the // CLDR spec? - fLastBaseUnit.clear(); - fLastBaseUnit.appendInvariantChars(baseUnit, lenBaseUnit, status); + 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(), fLastBaseUnit.data()) == 0) { + strcmp(outVector[i]->baseUnit.data(), lastBaseUnit.data()) == 0) { return; } } @@ -104,30 +104,14 @@ class ConversionRateDataSink : public ResourceSink { return; } else { cr->sourceUnit.append(source, lenSource, status); - cr->baseUnit.append(fLastBaseUnit.data(), fLastBaseUnit.length(), status); + cr->baseUnit.append(lastBaseUnit.data(), lastBaseUnit.length(), status); cr->factor.appendInvariantChars(factor, lenFactor, status); if (offset != NULL) cr->offset.appendInvariantChars(offset, lenOffset, status); } } - /** - * Returns the MeasureUnit that was the conversion base unit of the most - * recent call to put() - typically meaning the most recent call to - * ures_getAllItemsWithFallback(). - */ - MeasureUnit getLastBaseUnit(UErrorCode &status) { - return MeasureUnit::forIdentifier(fLastBaseUnit.data(), status); - } - private: MaybeStackVector &outVector; - - // TODO(review): felt like a hack: provides easy access to the most recent - // baseUnit. This hack is another point making me wonder if doing this - // ResourceSink thing is worthwhile. Functional style is not more verbose, - // and IMHO more readable than this object-based approach where the output - // seems/feels like a side-effect. - CharString fLastBaseUnit; }; /** From 2deeb28f3c51aa16e351e6c70ea547cb5cef6679 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Tue, 14 Apr 2020 16:33:59 +0200 Subject: [PATCH 12/22] Add getAllConversionRates(), drop selective code. --- icu4c/source/i18n/unitsdata.cpp | 136 ++++++------------- icu4c/source/i18n/unitsdata.h | 7 + icu4c/source/test/intltest/unitsdatatest.cpp | 39 ++++-- 3 files changed, 82 insertions(+), 100 deletions(-) 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()); + // 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()); - } + // // 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()); + // } } } From 8b76ef3b51cd73862b923a3295e081fc9669b052 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Tue, 14 Apr 2020 17:10:56 +0200 Subject: [PATCH 13/22] Change outVector from reference to pointer for clarity of lack of ownership. --- icu4c/source/i18n/unitsdata.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/icu4c/source/i18n/unitsdata.cpp b/icu4c/source/i18n/unitsdata.cpp index 6bebed26400..3e285b65bd6 100644 --- a/icu4c/source/i18n/unitsdata.cpp +++ b/icu4c/source/i18n/unitsdata.cpp @@ -35,9 +35,9 @@ class ConversionRateDataSink : public ResourceSink { /** * Constructor. * @param out The vector to which ConversionRateInfo instances are to be - * added. + * added. This vector must outlive the use of the ResourceSink. */ - explicit ConversionRateDataSink(MaybeStackVector &out) : outVector(out) {} + explicit ConversionRateDataSink(MaybeStackVector *out) : outVector(out) {} /** * Adds the conversion rate information found in value to the output vector. @@ -84,7 +84,7 @@ class ConversionRateDataSink : public ResourceSink { } // We don't have this ConversionRateInfo yet: add it. - ConversionRateInfo *cr = outVector.emplaceBack(); + ConversionRateInfo *cr = outVector->emplaceBack(); if (!cr) { status = U_MEMORY_ALLOCATION_ERROR; return; @@ -99,7 +99,7 @@ class ConversionRateDataSink : public ResourceSink { } private: - MaybeStackVector &outVector; + MaybeStackVector *outVector; }; } // namespace @@ -107,7 +107,7 @@ class ConversionRateDataSink : public ResourceSink { MaybeStackVector U_I18N_API getAllConversionRates(UErrorCode &status) { MaybeStackVector result; LocalUResourceBundlePointer unitsBundle(ures_openDirect(NULL, "units", &status)); - ConversionRateDataSink sink(result); + ConversionRateDataSink sink(&result); ures_getAllItemsWithFallback(unitsBundle.getAlias(), "convertUnits", sink, status); return result; } From 4bd6bbcee20a35bb7922a9ee59f664af4817ee6d Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Tue, 14 Apr 2020 20:59:41 +0200 Subject: [PATCH 14/22] Improve documentation comments. --- icu4c/source/i18n/unitsdata.cpp | 12 +++--------- icu4c/source/i18n/unitsdata.h | 24 +++++++++++++++--------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/icu4c/source/i18n/unitsdata.cpp b/icu4c/source/i18n/unitsdata.cpp index 3e285b65bd6..27990386e7d 100644 --- a/icu4c/source/i18n/unitsdata.cpp +++ b/icu4c/source/i18n/unitsdata.cpp @@ -20,15 +20,7 @@ namespace { /** * A ResourceSink that collects conversion rate information. * - * This class is for use by ures_getAllItemsWithFallback. Example code for - * collecting conversion info for "mile" and "foot" into conversionInfoOutput: - * - * UErrorCode status = U_ZERO_ERROR; - * ures_getByKey(unitsBundle, "convertUnits", &fillIn, &status); - * MaybeStackVector conversionInfoOutput; - * ConversionRateDataSink convertSink(conversionInfoOutput); - * ures_getAllItemsWithFallback(fillIn, "mile", convertSink, status); - * ures_getAllItemsWithFallback(fillIn, "foot", convertSink, status); + * This class is for use by ures_getAllItemsWithFallback. */ class ConversionRateDataSink : public ResourceSink { public: @@ -55,6 +47,8 @@ class ConversionRateDataSink : public ResourceSink { void put(const char *source, ResourceValue &value, UBool /*noFallback*/, UErrorCode &status) { if (U_FAILURE(status)) return; if (uprv_strcmp(source, "convertUnits") != 0) { + // This is very strict, however it is the cheapest way to be sure + // that with `value`, we're looking at the convertUnits table. status = U_ILLEGAL_ARGUMENT_ERROR; return; } diff --git a/icu4c/source/i18n/unitsdata.h b/icu4c/source/i18n/unitsdata.h index 87e49a8ec6f..5bba426862d 100644 --- a/icu4c/source/i18n/unitsdata.h +++ b/icu4c/source/i18n/unitsdata.h @@ -15,11 +15,15 @@ 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. +/** + * 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. Rationale: symbols can cancel out for higher + * precision conversion - going from feet to inches should cancel out the + * `ft_to_m` constant. + */ class U_I18N_API ConversionRateInfo { public: ConversionRateInfo(){}; @@ -45,11 +49,13 @@ class U_I18N_API ConversionRateInfo { MaybeStackVector U_I18N_API getAllConversionRates(UErrorCode &status); /** - * Collects and returns ConversionRateInfo needed for conversions for a set of - * units. + * Temporary backward-compatibility function. * - * @param units The units for which to load conversion data. - * @param status Receives status. + * TODO(hugovdm): ensure this gets removed. Currently + * https://github.com/sffc/icu/pull/32 is making use of it. + * + * @param units Ignored. + * @return the result of getAllConversionRates. */ MaybeStackVector U_I18N_API getConversionRatesInfo(const MaybeStackVector &units, UErrorCode &status); From 7b7df704513964311421eb0fd2fba936ced0ac88 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Wed, 15 Apr 2020 13:55:00 +0200 Subject: [PATCH 15/22] Return getAllConversionRates result via pointer parameter. --- icu4c/source/i18n/unitsdata.cpp | 16 +++++++++------- icu4c/source/i18n/unitsdata.h | 3 ++- icu4c/source/test/intltest/unitsdatatest.cpp | 3 ++- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/icu4c/source/i18n/unitsdata.cpp b/icu4c/source/i18n/unitsdata.cpp index 27990386e7d..b43d8be7a51 100644 --- a/icu4c/source/i18n/unitsdata.cpp +++ b/icu4c/source/i18n/unitsdata.cpp @@ -98,17 +98,19 @@ class ConversionRateDataSink : public ResourceSink { } // namespace -MaybeStackVector U_I18N_API getAllConversionRates(UErrorCode &status) { - MaybeStackVector result; +void U_I18N_API getAllConversionRates(MaybeStackVector *result, UErrorCode &status) { LocalUResourceBundlePointer unitsBundle(ures_openDirect(NULL, "units", &status)); - ConversionRateDataSink sink(&result); + ConversionRateDataSink sink(result); ures_getAllItemsWithFallback(unitsBundle.getAlias(), "convertUnits", sink, status); - return result; } -MaybeStackVector -U_I18N_API getConversionRatesInfo(const MaybeStackVector &, UErrorCode &status) { - return getAllConversionRates(status); +// TODO(hugovdm): ensure this gets removed. Currently +// https://github.com/sffc/icu/pull/32 is making use of it. +MaybeStackVector U_I18N_API +getConversionRatesInfo(const MaybeStackVector &, UErrorCode &status) { + MaybeStackVector result; + getAllConversionRates(&result, status); + return result; } U_NAMESPACE_END diff --git a/icu4c/source/i18n/unitsdata.h b/icu4c/source/i18n/unitsdata.h index 5bba426862d..59385c4a307 100644 --- a/icu4c/source/i18n/unitsdata.h +++ b/icu4c/source/i18n/unitsdata.h @@ -44,9 +44,10 @@ class U_I18N_API ConversionRateInfo { /** * Returns ConversionRateInfo for all supported conversions. * + * @param result Receives the set of conversion rates. * @param status Receives status. */ -MaybeStackVector U_I18N_API getAllConversionRates(UErrorCode &status); +void U_I18N_API getAllConversionRates(MaybeStackVector *result, UErrorCode &status); /** * Temporary backward-compatibility function. diff --git a/icu4c/source/test/intltest/unitsdatatest.cpp b/icu4c/source/test/intltest/unitsdatatest.cpp index 7c42e2ff5c5..bae722cb312 100644 --- a/icu4c/source/test/intltest/unitsdatatest.cpp +++ b/icu4c/source/test/intltest/unitsdatatest.cpp @@ -28,7 +28,8 @@ void UnitsDataTest::runIndexedTest(int32_t index, UBool exec, const char *&name, void UnitsDataTest::testGetAllConversionRates() { IcuTestErrorCode status(*this, "testGetAllConversionRates"); - MaybeStackVector conversionInfo = getAllConversionRates(status); + MaybeStackVector conversionInfo; + getAllConversionRates(&conversionInfo, status); // Convenience output for debugging for (int i = 0; i < conversionInfo.length(); i++) { From bb35a5a61eca2bf6a045692a1787999c21647cbc Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Wed, 15 Apr 2020 13:59:48 +0200 Subject: [PATCH 16/22] Switch result parameter from pointer to reference. --- icu4c/source/i18n/unitsdata.cpp | 6 +++--- icu4c/source/i18n/unitsdata.h | 2 +- icu4c/source/test/intltest/unitsdatatest.cpp | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/icu4c/source/i18n/unitsdata.cpp b/icu4c/source/i18n/unitsdata.cpp index b43d8be7a51..e4261ada3ab 100644 --- a/icu4c/source/i18n/unitsdata.cpp +++ b/icu4c/source/i18n/unitsdata.cpp @@ -98,9 +98,9 @@ class ConversionRateDataSink : public ResourceSink { } // namespace -void U_I18N_API getAllConversionRates(MaybeStackVector *result, UErrorCode &status) { +void U_I18N_API getAllConversionRates(MaybeStackVector &result, UErrorCode &status) { LocalUResourceBundlePointer unitsBundle(ures_openDirect(NULL, "units", &status)); - ConversionRateDataSink sink(result); + ConversionRateDataSink sink(&result); ures_getAllItemsWithFallback(unitsBundle.getAlias(), "convertUnits", sink, status); } @@ -109,7 +109,7 @@ void U_I18N_API getAllConversionRates(MaybeStackVector *resu MaybeStackVector U_I18N_API getConversionRatesInfo(const MaybeStackVector &, UErrorCode &status) { MaybeStackVector result; - getAllConversionRates(&result, status); + getAllConversionRates(result, status); return result; } diff --git a/icu4c/source/i18n/unitsdata.h b/icu4c/source/i18n/unitsdata.h index 59385c4a307..30ffc588241 100644 --- a/icu4c/source/i18n/unitsdata.h +++ b/icu4c/source/i18n/unitsdata.h @@ -47,7 +47,7 @@ class U_I18N_API ConversionRateInfo { * @param result Receives the set of conversion rates. * @param status Receives status. */ -void U_I18N_API getAllConversionRates(MaybeStackVector *result, UErrorCode &status); +void U_I18N_API getAllConversionRates(MaybeStackVector &result, UErrorCode &status); /** * Temporary backward-compatibility function. diff --git a/icu4c/source/test/intltest/unitsdatatest.cpp b/icu4c/source/test/intltest/unitsdatatest.cpp index bae722cb312..44d67031ed1 100644 --- a/icu4c/source/test/intltest/unitsdatatest.cpp +++ b/icu4c/source/test/intltest/unitsdatatest.cpp @@ -29,7 +29,7 @@ void UnitsDataTest::runIndexedTest(int32_t index, UBool exec, const char *&name, void UnitsDataTest::testGetAllConversionRates() { IcuTestErrorCode status(*this, "testGetAllConversionRates"); MaybeStackVector conversionInfo; - getAllConversionRates(&conversionInfo, status); + getAllConversionRates(conversionInfo, status); // Convenience output for debugging for (int i = 0; i < conversionInfo.length(); i++) { From 8b7e4b1d8aeeb714922db966645321366309b191 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Thu, 16 Apr 2020 03:59:44 +0200 Subject: [PATCH 17/22] Lint: add copyright notices to .txt files from CLDR. --- icu4c/source/test/testdata/units/unitPreferencesTest.txt | 3 +++ icu4c/source/test/testdata/units/unitsTest.txt | 3 +++ 2 files changed, 6 insertions(+) diff --git a/icu4c/source/test/testdata/units/unitPreferencesTest.txt b/icu4c/source/test/testdata/units/unitPreferencesTest.txt index 52673f99e0e..b34b7e21b91 100644 --- a/icu4c/source/test/testdata/units/unitPreferencesTest.txt +++ b/icu4c/source/test/testdata/units/unitPreferencesTest.txt @@ -1,3 +1,6 @@ +# Copyright (C) 2020 and later: Unicode, Inc. and others. +# License & terms of use: http://www.unicode.org/copyright.html +# # This file is a copy of common/testData/units/unitPreferencesTest.txt from CLDR. # WIP/TODO(hugovdm): determine a good update procedure and document it. diff --git a/icu4c/source/test/testdata/units/unitsTest.txt b/icu4c/source/test/testdata/units/unitsTest.txt index 0fe190a7074..614b82c4ffa 100644 --- a/icu4c/source/test/testdata/units/unitsTest.txt +++ b/icu4c/source/test/testdata/units/unitsTest.txt @@ -1,3 +1,6 @@ +# Copyright (C) 2020 and later: Unicode, Inc. and others. +# License & terms of use: http://www.unicode.org/copyright.html +# # This file is a copy of common/testData/units/unitsTest.txt from CLDR. # WIP/TODO(hugovdm): determine a good update procedure and document it. From 7f84bbaff9f87bfd3445ef59fab2d303122a19ab Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Thu, 16 Apr 2020 04:10:43 +0200 Subject: [PATCH 18/22] Comment out broken test cases as per younies' 476ca805c --- icu4c/source/test/intltest/unitstest.cpp | 218 +++++++++++------------ 1 file changed, 109 insertions(+), 109 deletions(-) diff --git a/icu4c/source/test/intltest/unitstest.cpp b/icu4c/source/test/intltest/unitstest.cpp index 41d761132fa..17aea41c57d 100644 --- a/icu4c/source/test/intltest/unitstest.cpp +++ b/icu4c/source/test/intltest/unitstest.cpp @@ -25,11 +25,11 @@ class UnitsTest : public IntlTest { void testConversions(); void testPreferences(); - void testBasic(); - void testSiPrefixes(); - void testMass(); - void testTemperature(); - void testArea(); + // void testBasic(); + // void testSiPrefixes(); + // void testMass(); + // void testTemperature(); + // void testArea(); }; extern IntlTest *createUnitsTest() { return new UnitsTest(); } @@ -41,11 +41,11 @@ void UnitsTest::runIndexedTest(int32_t index, UBool exec, const char *&name, cha TESTCASE_AUTO_BEGIN; TESTCASE_AUTO(testConversions); TESTCASE_AUTO(testPreferences); - TESTCASE_AUTO(testBasic); - TESTCASE_AUTO(testSiPrefixes); - TESTCASE_AUTO(testMass); - TESTCASE_AUTO(testTemperature); - TESTCASE_AUTO(testArea); + // TESTCASE_AUTO(testBasic); + // TESTCASE_AUTO(testSiPrefixes); + // TESTCASE_AUTO(testMass); + // TESTCASE_AUTO(testTemperature); + // TESTCASE_AUTO(testArea); TESTCASE_AUTO_END; } @@ -60,117 +60,117 @@ double testConvert(UnicodeString source, UnicodeString target, double input) { return -1; } -void UnitsTest::testBasic() { - IcuTestErrorCode status(*this, "Units testBasic"); +// void UnitsTest::testBasic() { +// IcuTestErrorCode status(*this, "Units testBasic"); - // Test Cases - struct TestCase { - const char16_t *source; - const char16_t *target; - const double inputValue; - const double expectedValue; - } testCases[]{{u"meter", u"foot", 1.0, 3.28084}, {u"kilometer", u"foot", 1.0, 328.084}}; +// // Test Cases +// struct TestCase { +// const char16_t *source; +// const char16_t *target; +// const double inputValue; +// const double expectedValue; +// } testCases[]{{u"meter", u"foot", 1.0, 3.28084}, {u"kilometer", u"foot", 1.0, 328.084}}; - for (const auto &testCase : testCases) { - assertEquals("test convert", testConvert(testCase.source, testCase.target, testCase.inputValue), - testCase.expectedValue); - } -} +// for (const auto &testCase : testCases) { +// assertEquals("test convert", testConvert(testCase.source, testCase.target, testCase.inputValue), +// testCase.expectedValue); +// } +// } -void UnitsTest::testSiPrefixes() { - IcuTestErrorCode status(*this, "Units testSiPrefixes"); - // Test Cases - struct TestCase { - const char16_t *source; - const char16_t *target; - const double inputValue; - const double expectedValue; - } testCases[]{ - {u"gram", u"kilogram", 1.0, 0.001}, // - {u"milligram", u"kilogram", 1.0, 0.000001}, // - {u"microgram", u"kilogram", 1.0, 0.000000001}, // - {u"megawatt", u"watt", 1, 1000000}, // - {u"megawatt", u"kilowatt", 1.0, 1000}, // - {u"gigabyte", u"byte", 1, 1000000000} // - }; +// void UnitsTest::testSiPrefixes() { +// IcuTestErrorCode status(*this, "Units testSiPrefixes"); +// // Test Cases +// struct TestCase { +// const char16_t *source; +// const char16_t *target; +// const double inputValue; +// const double expectedValue; +// } testCases[]{ +// {u"gram", u"kilogram", 1.0, 0.001}, // +// {u"milligram", u"kilogram", 1.0, 0.000001}, // +// {u"microgram", u"kilogram", 1.0, 0.000000001}, // +// {u"megawatt", u"watt", 1, 1000000}, // +// {u"megawatt", u"kilowatt", 1.0, 1000}, // +// {u"gigabyte", u"byte", 1, 1000000000} // +// }; - for (const auto &testCase : testCases) { - assertEquals("test convert", testConvert(testCase.source, testCase.target, testCase.inputValue), - testCase.expectedValue); - } -} +// for (const auto &testCase : testCases) { +// assertEquals("test convert", testConvert(testCase.source, testCase.target, testCase.inputValue), +// testCase.expectedValue); +// } +// } -void UnitsTest::testMass() { - IcuTestErrorCode status(*this, "Units testMass"); +// void UnitsTest::testMass() { +// IcuTestErrorCode status(*this, "Units testMass"); - // Test Cases - struct TestCase { - const char16_t *source; - const char16_t *target; - const double inputValue; - const double expectedValue; - } testCases[]{ - {u"gram", u"kilogram", 1.0, 0.001}, // - {u"pound", u"kilogram", 1.0, 0.453592}, // - {u"pound", u"kilogram", 2.0, 0.907185}, // - {u"ounce", u"pound", 16.0, 1.0}, // - {u"ounce", u"kilogram", 16.0, 0.453592}, // - {u"ton", u"pound", 1.0, 2000}, // - {u"stone", u"pound", 1.0, 14}, // - {u"stone", u"kilogram", 1.0, 6.35029} // - }; +// // Test Cases +// struct TestCase { +// const char16_t *source; +// const char16_t *target; +// const double inputValue; +// const double expectedValue; +// } testCases[]{ +// {u"gram", u"kilogram", 1.0, 0.001}, // +// {u"pound", u"kilogram", 1.0, 0.453592}, // +// {u"pound", u"kilogram", 2.0, 0.907185}, // +// {u"ounce", u"pound", 16.0, 1.0}, // +// {u"ounce", u"kilogram", 16.0, 0.453592}, // +// {u"ton", u"pound", 1.0, 2000}, // +// {u"stone", u"pound", 1.0, 14}, // +// {u"stone", u"kilogram", 1.0, 6.35029} // +// }; - for (const auto &testCase : testCases) { - assertEquals("test convert", testConvert(testCase.source, testCase.target, testCase.inputValue), - testCase.expectedValue); - } -} +// for (const auto &testCase : testCases) { +// assertEquals("test convert", testConvert(testCase.source, testCase.target, testCase.inputValue), +// testCase.expectedValue); +// } +// } -void UnitsTest::testTemperature() { - IcuTestErrorCode status(*this, "Units testTemperature"); - // Test Cases - struct TestCase { - const char16_t *source; - const char16_t *target; - const double inputValue; - const double expectedValue; - } testCases[]{ - {u"celsius", u"fahrenheit", 0.0, 32.0}, // - {u"celsius", u"fahrenheit", 10.0, 50.0}, // - {u"fahrenheit", u"celsius", 32.0, 0.0}, // - {u"fahrenheit", u"celsius", 89.6, 32}, // - {u"kelvin", u"fahrenheit", 0.0, -459.67}, // - {u"kelvin", u"fahrenheit", 300, 80.33}, // - {u"kelvin", u"celsius", 0.0, -273.15}, // - {u"kelvin", u"celsius", 300.0, 26.85} // - }; +// void UnitsTest::testTemperature() { +// IcuTestErrorCode status(*this, "Units testTemperature"); +// // Test Cases +// struct TestCase { +// const char16_t *source; +// const char16_t *target; +// const double inputValue; +// const double expectedValue; +// } testCases[]{ +// {u"celsius", u"fahrenheit", 0.0, 32.0}, // +// {u"celsius", u"fahrenheit", 10.0, 50.0}, // +// {u"fahrenheit", u"celsius", 32.0, 0.0}, // +// {u"fahrenheit", u"celsius", 89.6, 32}, // +// {u"kelvin", u"fahrenheit", 0.0, -459.67}, // +// {u"kelvin", u"fahrenheit", 300, 80.33}, // +// {u"kelvin", u"celsius", 0.0, -273.15}, // +// {u"kelvin", u"celsius", 300.0, 26.85} // +// }; - for (const auto &testCase : testCases) { - assertEquals("test convert", testConvert(testCase.source, testCase.target, testCase.inputValue), - testCase.expectedValue); - } -} +// for (const auto &testCase : testCases) { +// assertEquals("test convert", testConvert(testCase.source, testCase.target, testCase.inputValue), +// testCase.expectedValue); +// } +// } -void UnitsTest::testArea() { - IcuTestErrorCode status(*this, "Units Area"); +// void UnitsTest::testArea() { +// IcuTestErrorCode status(*this, "Units Area"); - // Test Cases - struct TestCase { - const char16_t *source; - const char16_t *target; - const double inputValue; - const double expectedValue; - } testCases[]{ - {u"square-meter", u"square-yard", 10.0, 11.9599}, // - {u"hectare", u"square-yard", 1.0, 11959.9}, // - {u"square-mile", u"square-foot", 0.0001, 2787.84} // - }; +// // Test Cases +// struct TestCase { +// const char16_t *source; +// const char16_t *target; +// const double inputValue; +// const double expectedValue; +// } testCases[]{ +// {u"square-meter", u"square-yard", 10.0, 11.9599}, // +// {u"hectare", u"square-yard", 1.0, 11959.9}, // +// {u"square-mile", u"square-foot", 0.0001, 2787.84} // +// }; - for (const auto &testCase : testCases) { - assertEquals("test convert", testConvert(testCase.source, testCase.target, testCase.inputValue), - testCase.expectedValue); - } -} +// for (const auto &testCase : testCases) { +// assertEquals("test convert", testConvert(testCase.source, testCase.target, testCase.inputValue), +// testCase.expectedValue); +// } +// } /** * Trims whitespace (spaces only) off of the specified string. From a5deb5550c6f4c6da08e666aeaa9e9d55f715feb Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Fri, 17 Apr 2020 00:47:30 +0200 Subject: [PATCH 19/22] Nit: remove two unneeded #includes --- icu4c/source/i18n/unitsdata.cpp | 2 -- icu4c/source/i18n/unitsdata.h | 1 - 2 files changed, 3 deletions(-) diff --git a/icu4c/source/i18n/unitsdata.cpp b/icu4c/source/i18n/unitsdata.cpp index e4261ada3ab..5ad29217e3c 100644 --- a/icu4c/source/i18n/unitsdata.cpp +++ b/icu4c/source/i18n/unitsdata.cpp @@ -5,8 +5,6 @@ #if !UCONFIG_NO_FORMATTING -#include - #include "cstring.h" #include "resource.h" #include "unitsdata.h" diff --git a/icu4c/source/i18n/unitsdata.h b/icu4c/source/i18n/unitsdata.h index 30ffc588241..6c6154ae8a4 100644 --- a/icu4c/source/i18n/unitsdata.h +++ b/icu4c/source/i18n/unitsdata.h @@ -11,7 +11,6 @@ #include "cmemory.h" #include "unicode/measunit.h" #include "unicode/stringpiece.h" -#include "unicode/utypes.h" U_NAMESPACE_BEGIN From 1461ad48473a7e51e9b9e540cca0fb9627f5b6c8 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Fri, 17 Apr 2020 00:49:06 +0200 Subject: [PATCH 20/22] dependencies.txt attempt #1: where's "operator new(unsigned long)" coming from? --- icu4c/source/test/depstest/dependencies.txt | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/icu4c/source/test/depstest/dependencies.txt b/icu4c/source/test/depstest/dependencies.txt index e096586b3f3..d74e548026c 100644 --- a/icu4c/source/test/depstest/dependencies.txt +++ b/icu4c/source/test/depstest/dependencies.txt @@ -870,7 +870,7 @@ library: i18n listformatter formatting formattable_cnv regex regex_cnv translit double_conversion number_representation number_output numberformatter number_skeletons numberparser - units_extra + units_extra unitsformatter universal_time_scale uclean_i18n @@ -1063,7 +1063,7 @@ group: sharedbreakiterator breakiterator group: units_extra - measunit_extra.o + measunit_extra.o unitconverter.o deps units ucharstriebuilder ucharstrie uclean_i18n @@ -1072,6 +1072,11 @@ group: units deps stringenumeration errorcode +group: unitsformatter + unitsdata.o + deps + resourcebundle + group: decnumber decContext.o decNumber.o deps From 2639dc3670b338a3b35770df8733c89d68cae50a Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Fri, 17 Apr 2020 01:47:57 +0200 Subject: [PATCH 21/22] Fix deps: use UMemory as base class for it's new(). --- icu4c/source/i18n/unitsdata.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/icu4c/source/i18n/unitsdata.h b/icu4c/source/i18n/unitsdata.h index 6c6154ae8a4..5f4306dc012 100644 --- a/icu4c/source/i18n/unitsdata.h +++ b/icu4c/source/i18n/unitsdata.h @@ -23,7 +23,7 @@ U_NAMESPACE_BEGIN * precision conversion - going from feet to inches should cancel out the * `ft_to_m` constant. */ -class U_I18N_API ConversionRateInfo { +class U_I18N_API ConversionRateInfo : public UMemory { public: ConversionRateInfo(){}; ConversionRateInfo(StringPiece sourceUnit, StringPiece baseUnit, StringPiece factor, From 8497f0783ae250f6802db220822c899cc37589a6 Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Fri, 17 Apr 2020 01:49:29 +0200 Subject: [PATCH 22/22] Remove testGetConversionRateInfo. --- icu4c/source/test/intltest/unitsdatatest.cpp | 87 -------------------- 1 file changed, 87 deletions(-) diff --git a/icu4c/source/test/intltest/unitsdatatest.cpp b/icu4c/source/test/intltest/unitsdatatest.cpp index 44d67031ed1..817082212a7 100644 --- a/icu4c/source/test/intltest/unitsdatatest.cpp +++ b/icu4c/source/test/intltest/unitsdatatest.cpp @@ -13,7 +13,6 @@ class UnitsDataTest : public IntlTest { void runIndexedTest(int32_t index, UBool exec, const char *&name, char *par = NULL); void testGetAllConversionRates(); - void testGetConversionRateInfo(); }; extern IntlTest *createUnitsDataTest() { return new UnitsDataTest(); } @@ -22,7 +21,6 @@ void UnitsDataTest::runIndexedTest(int32_t index, UBool exec, const char *&name, if (exec) { logln("TestSuite UnitsDataTest: "); } TESTCASE_AUTO_BEGIN; TESTCASE_AUTO(testGetAllConversionRates); - TESTCASE_AUTO(testGetConversionRateInfo); TESTCASE_AUTO_END; } @@ -42,89 +40,4 @@ void UnitsDataTest::testGetAllConversionRates() { } } -// 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 { - // The source unit passed to getConversionRateInfo. - const char *sourceUnit; - // The target unit passed to getConversionRateInfo. - const char *targetUnit; - // Expected: units whose conversion rates are expected in the results. - const char *expectedOutputs[MAX_NUM_RATES]; - } testCases[]{ - {"meter", "kilometer", {"meter", NULL, NULL, NULL, NULL}}, - {"liter", "gallon", {"liter", "gallon", NULL, NULL, NULL}}, - {"watt", "horsepower", {"watt", "horsepower", NULL, NULL, NULL}}, - {"mile-per-hour", "dekameter-per-hour", {"mile", "hour", "meter", NULL, NULL}}, - - // Sequence - {"stone-and-pound", "ton", {"pound", "stone", "ton", NULL, NULL}}, - - // Compound - {"centimeter-per-square-milligram", - "inch-per-square-ounce", - {"meter", "gram", "inch", "ounce", NULL}}, - {"therm-us", - "kilogram-square-meter-per-square-second", - {"therm-us", "kilogram", "meter", "second", NULL}}, - - // "Reciprocal" example: consumption and consumption-inverse - {"liter-per-100-kilometer", - "mile-per-gallon", - {"liter", "100-kilometer", "mile", "gallon", NULL}}, - }; - for (const auto &t : testCases) { - logln("---testing: source=\"%s\", target=\"%s\"", t.sourceUnit, t.targetUnit); - IcuTestErrorCode status(*this, "testGetConversionRateInfo"); - - MaybeStackVector units; - MeasureUnit *item = units.emplaceBack(MeasureUnit::forIdentifier(t.sourceUnit, status)); - if (!item) { - status.set(U_MEMORY_ALLOCATION_ERROR); - return; - } - item = units.emplaceBack(MeasureUnit::forIdentifier(t.targetUnit, status)); - if (!item) { - status.set(U_MEMORY_ALLOCATION_ERROR); - return; - } - - MaybeStackVector conversionInfo = getConversionRatesInfo(units, status); - if (status.errIfFailureAndReset("getConversionRatesInfo({<%s>, <%s>}, ...)", t.sourceUnit, - t.targetUnit)) { - continue; - } - - int countExpected; - for (countExpected = 0; countExpected < MAX_NUM_RATES; countExpected++) { - auto expected = t.expectedOutputs[countExpected]; - if (expected == NULL) break; - // Check if this conversion rate was expected - bool found = false; - for (int i = 0; i < conversionInfo.length(); i++) { - auto cri = conversionInfo[i]; - if (strcmp(expected, cri->sourceUnit.data()) == 0) { - found = true; - break; - } - } - assertTrue(UnicodeString("<") + expected + "> expected", found); - } - // 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()); - // } - } -} - #endif /* #if !UCONFIG_NO_FORMATTING */