From 7b05da09108883c0f2b545cccca916f0edbb464d Mon Sep 17 00:00:00 2001 From: Hugo van der Merwe <17109322+hugovdm@users.noreply.github.com> Date: Mon, 29 Nov 2021 20:01:31 +0100 Subject: [PATCH] ICU-21863 Fix div-by-zero in ICU4J, test inverse unit conversions Also cleans up some old icu-units TODOs: - This PR fixes icu-units#38 and icu-units#63 TODOs (now part of ICU-21862) - icu-units#21 is obsolete --- icu4c/source/i18n/units_converter.cpp | 6 ++--- icu4c/source/i18n/units_router.h | 2 -- icu4c/source/test/intltest/numbertest_api.cpp | 26 ++++++++++++++++--- icu4c/source/test/intltest/units_test.cpp | 17 +++++++++--- .../ibm/icu/impl/units/UnitsConverter.java | 12 +++------ .../com/ibm/icu/dev/test/impl/UnitsTest.java | 20 +++++++++----- .../test/number/NumberFormatterApiTest.java | 26 ++++++++++++++++--- 7 files changed, 77 insertions(+), 32 deletions(-) diff --git a/icu4c/source/i18n/units_converter.cpp b/icu4c/source/i18n/units_converter.cpp index 7e946e584bb..d8ef596d735 100644 --- a/icu4c/source/i18n/units_converter.cpp +++ b/icu4c/source/i18n/units_converter.cpp @@ -603,9 +603,9 @@ double UnitsConverter::convertInverse(double inputValue) const { double result = inputValue; if (conversionRate_.reciprocal) { if (result == 0) { - // TODO: demonstrate the resulting behaviour in tests... and figure - // out desired behaviour. (Theoretical result should be infinity, - // not 0.) + // TODO(ICU-21862): demonstrate the resulting behaviour in tests... + // and figure out desired behaviour. (Theoretical result should be + // infinity, not 0.) return 0.0; } result = 1.0 / result; diff --git a/icu4c/source/i18n/units_router.h b/icu4c/source/i18n/units_router.h index b3300f7e277..d9fcffb2aa9 100644 --- a/icu4c/source/i18n/units_router.h +++ b/icu4c/source/i18n/units_router.h @@ -30,8 +30,6 @@ namespace units { struct RouteResult : UMemory { // A list of measures: a single measure for single units, multiple measures // for mixed units. - // - // TODO(icu-units/icu#21): figure out the right mixed unit API. MaybeStackVector measures; // The output unit for this RouteResult. This may be a MIXED unit - for diff --git a/icu4c/source/test/intltest/numbertest_api.cpp b/icu4c/source/test/intltest/numbertest_api.cpp index cf8fecc0a28..c933936b752 100644 --- a/icu4c/source/test/intltest/numbertest_api.cpp +++ b/icu4c/source/test/intltest/numbertest_api.cpp @@ -1747,6 +1747,28 @@ void NumberFormatterApiTest::unitUsage() { 30500, u"350 m"); + assertFormatSingle(u"Fuel consumption: inverted units", // + u"unit/liter-per-100-kilometer usage/vehicle-fuel", // + u"unit/liter-per-100-kilometer usage/vehicle-fuel", // + NumberFormatter::with() // + .unit(MeasureUnit::forIdentifier("liter-per-100-kilometer", status)) // + .usage("vehicle-fuel"), // + Locale("en-US"), // + 6.6, // + "36 mpg"); + +// // TODO(ICU-21862): determine desired behaviour. Commented out for now to not enforce undesirable +// // behaviour +// assertFormatSingle(u"Fuel consumption: inverted units, divide-by-zero", // +// u"unit/liter-per-100-kilometer usage/vehicle-fuel", // +// u"unit/liter-per-100-kilometer usage/vehicle-fuel", // +// NumberFormatter::with() // +// .unit(MeasureUnit::forIdentifier("liter-per-100-kilometer", status)) // +// .usage("vehicle-fuel"), // +// Locale("en-US"), // +// 0, // +// "0 mpg"); // TODO(ICU-21862) + // Test calling `.usage("")` should unset the existing usage. // First: without usage assertFormatSingle(u"Rounding Mode propagates: rounding up", @@ -1853,10 +1875,6 @@ void NumberFormatterApiTest::unitUsage() { Locale("en-US"), // 1, // "0.019 psi"); - - // TODO(icu-units#38): improve unit testing coverage. E.g. add vehicle-fuel - // triggering inversion conversion code. Test with 0 too, to see - // divide-by-zero behaviour. } void NumberFormatterApiTest::unitUsageErrorCodes() { diff --git a/icu4c/source/test/intltest/units_test.cpp b/icu4c/source/test/intltest/units_test.cpp index c65a5e03229..88b677c60e0 100644 --- a/icu4c/source/test/intltest/units_test.cpp +++ b/icu4c/source/test/intltest/units_test.cpp @@ -325,6 +325,9 @@ void UnitsTest::testConverter() { // Fuel Consumption {"cubic-meter-per-meter", "mile-per-gallon", 2.1383143939394E-6, 1.1}, {"cubic-meter-per-meter", "mile-per-gallon", 2.6134953703704E-6, 0.9}, + {"liter-per-100-kilometer", "mile-per-gallon", 6.6, 35.6386}, + // // TODO(ICU-21862): we should probably return something other than "0": + // {"liter-per-100-kilometer", "mile-per-gallon", 0, 0}, // Test Aliases // Alias is just another name to the same unit. Therefore, converting @@ -647,6 +650,16 @@ void UnitsTest::testComplexUnitsConverter() { Measure(2.1, MeasureUnit::createMeter(status), status)}, 2, 0.001}, + + // Negative numbers + {"Negative number conversion", + "yard", + "mile-and-yard", + -1800, + {Measure(-1, MeasureUnit::createMile(status), status), + Measure(-40, MeasureUnit::createYard(status), status)}, + 2, + 1e-10}, }; status.assertSuccess(); @@ -694,11 +707,7 @@ void UnitsTest::testComplexUnitsConverter() { ComplexUnitsConverter converter2( testCase.input, testCase.output, status); testATestCase(converter2, "ComplexUnitsConverter #1 " , testCase); } - - status.assertSuccess(); - - // TODO(icu-units#63): test negative numbers! } void UnitsTest::testComplexUnitsConverterSorting() { diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsConverter.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsConverter.java index acece19279e..156a6e0b6db 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsConverter.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/units/UnitsConverter.java @@ -115,10 +115,8 @@ public class UnitsConverter { if (this.reciprocal) { // We should see no offsets for reciprocal conversions - they don't make sense: assert offset == BigDecimal.ZERO; - if (result == BigDecimal.ZERO) { - // TODO: demonstrate the resulting behaviour in tests... and - // figure out desired behaviour. (Theoretical result should be - // infinity, not 0, but BigDecimal does not support infinity.) + if (result.compareTo(BigDecimal.ZERO) == 0) { + // TODO(ICU-21862): determine desirable behaviour return BigDecimal.ZERO; } result = BigDecimal.ONE.divide(result, DECIMAL128); @@ -131,10 +129,8 @@ public class UnitsConverter { if (this.reciprocal) { // We should see no offsets for reciprocal conversions - they don't make sense: assert offset == BigDecimal.ZERO; - if (result == BigDecimal.ZERO) { - // TODO: demonstrate the resulting behaviour in tests... and - // figure out desired behaviour. (Theoretical result should be - // infinity, not 0, but BigDecimal does not support infinity.) + if (result.compareTo(BigDecimal.ZERO) == 0) { + // TODO(ICU-21862): determine desirable behaviour return BigDecimal.ZERO; } result = BigDecimal.ONE.divide(result, DECIMAL128); diff --git a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/impl/UnitsTest.java b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/impl/UnitsTest.java index c8706cbbfc4..1502fe7ecdc 100644 --- a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/impl/UnitsTest.java +++ b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/impl/UnitsTest.java @@ -90,7 +90,7 @@ public class UnitsTest { 0), // A minimal nudge under 2.0, rounding up to 2.0 ft, 0 in. - // TODO(icu-units#108): this matches double precision calculations + // TODO(ICU-21861): this matches double precision calculations // from C++, but BigDecimal is in use: do we want Java to be more // precise than C++? new TestCase( @@ -116,7 +116,7 @@ public class UnitsTest { // 1e-16 light years is 0.946073 meters. // A 2.1 meter nudge under 2.0 light years, rounding up to 2.0 ly, 0 m. - // TODO(icu-units#108): this matches double precision calculations + // TODO(ICU-21861): this matches double precision calculations // from C++, but BigDecimal is in use: do we want Java to be more // precise than C++? new TestCase("light-year", "light-year-and-meter", @@ -125,7 +125,7 @@ public class UnitsTest { new Measure(0, MeasureUnit.METER)}, 0), - // // TODO(icu-units#108): figure out precision thresholds for BigDecimal? + // // TODO(ICU-21861): figure out precision thresholds for BigDecimal? // // This test passes in C++ due to double-precision rounding. // // A 2.1 meter nudge under 1.0 light years, rounding up to 1.0 ly, 0 m. // new TestCase("light-year", "light-year-and-meter", @@ -143,7 +143,7 @@ public class UnitsTest { new Measure(9.46073, MeasureUnit.METER)}, 0 /* meters, precision */), - // TODO(icu-units#108): reconsider whether epsilon rounding is desirable: + // TODO(ICU-21861): reconsider whether epsilon rounding is desirable: // // 2e-16 light years is 1.892146 meters. For C++ double, we consider // this in the noise, and thus expect a 0. (This test fails when @@ -153,8 +153,13 @@ public class UnitsTest { new Measure[] {new Measure(1, MeasureUnit.LIGHT_YEAR), new Measure(1.892146, MeasureUnit.METER)}, 0), - }; + // Negative numbers + new TestCase( + "yard", "mile-and-yard", BigDecimal.valueOf(-1800), + new Measure[] {new Measure(-1, MeasureUnit.MILE), new Measure(-40, MeasureUnit.YARD)}, + 1e-10), + }; ConversionRates rates = new ConversionRates(); MeasureUnit input, output; @@ -171,8 +176,6 @@ public class UnitsTest { ComplexUnitsConverter converter2 = new ComplexUnitsConverter(testCase.input, testCase.output); testCase.testATestCase(converter2); } - - // TODO(icu-units#63): test negative numbers! } @@ -468,6 +471,9 @@ public class UnitsTest { // Fuel Consumption new TestData("cubic-meter-per-meter", "mile-per-gallon", 2.1383143939394E-6, 1.1), new TestData("cubic-meter-per-meter", "mile-per-gallon", 2.6134953703704E-6, 0.9), + new TestData("liter-per-100-kilometer", "mile-per-gallon", 6.6, 35.6386), + // // TODO(ICU-21862): we should probably return something other than "0": + // new TestData("liter-per-100-kilometer", "mile-per-gallon", 0, 0), // Test Aliases // Alias is just another name to the same unit. Therefore, converting // between them should be the same. diff --git a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java index c0ab0dadeb3..ee57c30f5ef 100644 --- a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java +++ b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberFormatterApiTest.java @@ -1671,6 +1671,28 @@ public class NumberFormatterApiTest extends TestFmwk { 30500, "350 m"); + assertFormatSingle("Fuel consumption: inverted units", + "unit/liter-per-100-kilometer usage/vehicle-fuel", + "unit/liter-per-100-kilometer usage/vehicle-fuel", + NumberFormatter.with() + .unit(MeasureUnit.forIdentifier("liter-per-100-kilometer")) + .usage("vehicle-fuel"), + new ULocale("en-US"), // + 6.6, // + "36 mpg"); + + // // TODO(ICU-21862): determine desired behaviour. Commented out for now + // // to not enforce undesirable behaviour + // assertFormatSingle("Fuel consumption: inverted units, divide-by-zero", + // "unit/liter-per-100-kilometer usage/vehicle-fuel", + // "unit/liter-per-100-kilometer usage/vehicle-fuel", + // NumberFormatter.with() + // .unit(MeasureUnit.forIdentifier("liter-per-100-kilometer")) + // .usage("vehicle-fuel"), + // new ULocale("en-US"), // + // 0, // + // "0 mpg"); + // Test calling .usage("") or .usage(null) should unset the existing usage. // First: without usage assertFormatSingle("Rounding Mode propagates: rounding up", @@ -1790,10 +1812,6 @@ public class NumberFormatterApiTest extends TestFmwk { new ULocale("en-US"), 1, "0.019 psi"); - - // TODO(icu-units#38): improve unit testing coverage. E.g. add - // vehicle-fuel triggering inversion conversion code. Test with 0 too, - // to see divide-by-zero behaviour. } @Test -- 2.40.0