From 29937704cda006f558b4ed06b3accfda148c56c0 Mon Sep 17 00:00:00 2001 From: Shane Carr Date: Wed, 30 May 2018 03:34:41 +0000 Subject: [PATCH] ICU-8610 Responding to number skeleton code review feedback. X-SVN-Rev: 41483 --- icu4c/source/i18n/number_skeletons.cpp | 43 ++++++++++++++----- icu4c/source/i18n/number_skeletons.h | 2 +- .../test/intltest/numbertest_skeletons.cpp | 15 ++++--- .../ibm/icu/number/NumberSkeletonImpl.java | 2 +- 4 files changed, 44 insertions(+), 18 deletions(-) diff --git a/icu4c/source/i18n/number_skeletons.cpp b/icu4c/source/i18n/number_skeletons.cpp index db2aabcffb6..5d74f1e6ae6 100644 --- a/icu4c/source/i18n/number_skeletons.cpp +++ b/icu4c/source/i18n/number_skeletons.cpp @@ -407,25 +407,21 @@ enum_to_stem_string::decimalSeparatorDisplay(UNumberDecimalSeparatorDisplay valu UnlocalizedNumberFormatter skeleton::create(const UnicodeString& skeletonString, UErrorCode& status) { - if (U_FAILURE(status)) { return {}; } umtx_initOnce(gNumberSkeletonsInitOnce, &initNumberSkeletons, status); - if (U_FAILURE(status)) { return {}; } - MacroProps macros = parseSkeleton(skeletonString, status); return NumberFormatter::with().macros(macros); } UnicodeString skeleton::generate(const MacroProps& macros, UErrorCode& status) { - if (U_FAILURE(status)) { return {}; } umtx_initOnce(gNumberSkeletonsInitOnce, &initNumberSkeletons, status); - if (U_FAILURE(status)) { return {}; } - UnicodeString sb; GeneratorHelpers::generateSkeleton(macros, sb, status); return sb; } MacroProps skeleton::parseSkeleton(const UnicodeString& skeletonString, UErrorCode& status) { + if (U_FAILURE(status)) { return {}; } + // Add a trailing whitespace to the end of the skeleton string to make code cleaner. UnicodeString tempSkeletonString(skeletonString); tempSkeletonString.append(u' '); @@ -713,9 +709,15 @@ ParseState skeleton::parseOption(ParseState stem, const StringSegment& segment, if (blueprint_helpers::parseExponentWidthOption(segment, macros, status)) { return STATE_SCIENTIFIC; } + if (U_FAILURE(status)) { + return {}; + } if (blueprint_helpers::parseExponentSignOption(segment, macros, status)) { return STATE_SCIENTIFIC; } + if (U_FAILURE(status)) { + return {}; + } break; default: break; @@ -727,6 +729,9 @@ ParseState skeleton::parseOption(ParseState stem, const StringSegment& segment, if (blueprint_helpers::parseFracSigOption(segment, macros, status)) { return STATE_NULL; } + if (U_FAILURE(status)) { + return {}; + } break; default: break; @@ -739,6 +744,8 @@ ParseState skeleton::parseOption(ParseState stem, const StringSegment& segment, } void GeneratorHelpers::generateSkeleton(const MacroProps& macros, UnicodeString& sb, UErrorCode& status) { + if (U_FAILURE(status)) { return; } + // Supported options if (GeneratorHelpers::notation(macros, sb, status)) { sb.append(u' '); @@ -902,7 +909,7 @@ void blueprint_helpers::parseMeasureUnitOption(const StringSegment& segment, Mac } // Need to do char <-> UChar conversion... - if (U_FAILURE(status)) { return; } + U_ASSERT(U_SUCCESS(status)); CharString type; SKELETON_UCHAR_TO_CHAR(type, stemString, 0, firstHyphen, status); CharString subType; @@ -944,6 +951,7 @@ void blueprint_helpers::parseMeasurePerUnitOption(const StringSegment& segment, // parsing code, put back the numerator unit, and put the new unit into per-unit. MeasureUnit numerator = macros.unit; parseMeasureUnitOption(segment, macros, status); + if (U_FAILURE(status)) { return; } macros.perUnit = macros.unit; macros.unit = numerator; } @@ -1039,6 +1047,7 @@ blueprint_helpers::parseDigitsStem(const StringSegment& segment, MacroProps& mac if (offset < segment.length()) { // throw new SkeletonSyntaxException("Invalid significant digits stem", segment); status = U_NUMBER_SKELETON_SYNTAX_ERROR; + return; } // Use the public APIs to enforce bounds checking if (maxSig == -1) { @@ -1121,6 +1130,7 @@ bool blueprint_helpers::parseFracSigOption(const StringSegment& segment, MacroPr void blueprint_helpers::parseIncrementOption(const StringSegment& segment, MacroProps& macros, UErrorCode& status) { // Need to do char <-> UChar conversion... + U_ASSERT(U_SUCCESS(status)); CharString buffer; SKELETON_UCHAR_TO_CHAR(buffer, segment.toTempUnicodeString(), 0, segment.length(), status); @@ -1218,11 +1228,13 @@ void blueprint_helpers::generateIntegerWidthOption(int32_t minInt, int32_t maxIn void blueprint_helpers::parseNumberingSystemOption(const StringSegment& segment, MacroProps& macros, UErrorCode& status) { // Need to do char <-> UChar conversion... + U_ASSERT(U_SUCCESS(status)); CharString buffer; SKELETON_UCHAR_TO_CHAR(buffer, segment.toTempUnicodeString(), 0, segment.length(), status); NumberingSystem* ns = NumberingSystem::createInstanceByName(buffer.data(), status); - if (ns == nullptr) { + if (ns == nullptr || U_FAILURE(status)) { + // This is a skeleton syntax error; don't bubble up the low-level NumberingSystem error // throw new SkeletonSyntaxException("Unknown numbering system", segment); status = U_NUMBER_SKELETON_SYNTAX_ERROR; return; @@ -1239,6 +1251,7 @@ void blueprint_helpers::generateNumberingSystemOption(const NumberingSystem& ns, void blueprint_helpers::parseScaleOption(const StringSegment& segment, MacroProps& macros, UErrorCode& status) { // Need to do char <-> UChar conversion... + U_ASSERT(U_SUCCESS(status)); CharString buffer; SKELETON_UCHAR_TO_CHAR(buffer, segment.toTempUnicodeString(), 0, segment.length(), status); @@ -1246,6 +1259,7 @@ void blueprint_helpers::parseScaleOption(const StringSegment& segment, MacroProp if (U_FAILURE(status)) { return; } decnum->setTo({buffer.data(), buffer.length()}, status); if (U_FAILURE(status)) { + // This is a skeleton syntax error; don't let the low-level decnum error bubble up status = U_NUMBER_SKELETON_SYNTAX_ERROR; return; } @@ -1254,13 +1268,13 @@ void blueprint_helpers::parseScaleOption(const StringSegment& segment, MacroProp macros.scale = {0, decnum.orphan()}; } -void -blueprint_helpers::generateScaleOption(int32_t magnitude, const DecNum* arbitrary, UnicodeString& sb, +void blueprint_helpers::generateScaleOption(int32_t magnitude, const DecNum* arbitrary, UnicodeString& sb, UErrorCode& status) { // Utilize DecimalQuantity/double_conversion to format this for us. DecimalQuantity dq; if (arbitrary != nullptr) { dq.setToDecNum(*arbitrary, status); + if (U_FAILURE(status)) { return; } } else { dq.setToInt(1); } @@ -1295,6 +1309,9 @@ bool GeneratorHelpers::notation(const MacroProps& macros, UnicodeString& sb, UEr if (impl.fMinExponentDigits > 1) { sb.append(u'/'); blueprint_helpers::generateExponentWidthOption(impl.fMinExponentDigits, sb, status); + if (U_FAILURE(status)) { + return false; + } } if (impl.fExponentSignDisplay != UNUM_SIGN_AUTO) { sb.append(u'/'); @@ -1310,7 +1327,11 @@ bool GeneratorHelpers::notation(const MacroProps& macros, UnicodeString& sb, UEr bool GeneratorHelpers::unit(const MacroProps& macros, UnicodeString& sb, UErrorCode& status) { if (utils::unitIsCurrency(macros.unit)) { sb.append(u"currency/", -1); - blueprint_helpers::generateCurrencyOption({macros.unit, status}, sb, status); + CurrencyUnit currency(macros.unit, status); + if (U_FAILURE(status)) { + return false; + } + blueprint_helpers::generateCurrencyOption(currency, sb, status); return true; } else if (utils::unitIsNoUnit(macros.unit)) { if (utils::unitIsPercent(macros.unit)) { diff --git a/icu4c/source/i18n/number_skeletons.h b/icu4c/source/i18n/number_skeletons.h index 3e4def26527..0161f5f0ba8 100644 --- a/icu4c/source/i18n/number_skeletons.h +++ b/icu4c/source/i18n/number_skeletons.h @@ -141,7 +141,7 @@ UnicodeString generate(const MacroProps& macros, UErrorCode& status); MacroProps parseSkeleton(const UnicodeString& skeletonString, UErrorCode& status); /** - * Given that the current segment represents an stem, parse it and save the result. + * Given that the current segment represents a stem, parse it and save the result. * * @return The next state after parsing this stem, corresponding to what subset of options to expect. */ diff --git a/icu4c/source/test/intltest/numbertest_skeletons.cpp b/icu4c/source/test/intltest/numbertest_skeletons.cpp index 9ee2325710f..6165daf4706 100644 --- a/icu4c/source/test/intltest/numbertest_skeletons.cpp +++ b/icu4c/source/test/intltest/numbertest_skeletons.cpp @@ -31,6 +31,8 @@ void NumberSkeletonTest::runIndexedTest(int32_t index, UBool exec, const char*& } void NumberSkeletonTest::validTokens() { + IcuTestErrorCode status(*this, "validTokens"); + // This tests only if the tokens are valid, not their behavior. // Most of these are from the design doc. static const char16_t* cases[] = { @@ -105,9 +107,10 @@ void NumberSkeletonTest::validTokens() { for (auto& cas : cases) { UnicodeString skeletonString(cas); - UErrorCode status = U_ZERO_ERROR; + status.setScope(skeletonString); NumberFormatter::forSkeleton(skeletonString, status); assertSuccess(skeletonString, status); + status.errIfFailureAndReset(); } } @@ -144,7 +147,7 @@ void NumberSkeletonTest::invalidTokens() { u"integer-width/+0#", u"scientific/foo"}; - expectedErrorSkeleton(cases, sizeof(cases) / sizeof(*cases)); + expectedErrorSkeleton(cases, UPRV_LENGTHOF(cases)); } void NumberSkeletonTest::unknownTokens() { @@ -157,7 +160,7 @@ void NumberSkeletonTest::unknownTokens() { u"numbering-system/français", // non-invariant characters for C++ u"currency-USD"}; - expectedErrorSkeleton(cases, sizeof(cases) / sizeof(*cases)); + expectedErrorSkeleton(cases, UPRV_LENGTHOF(cases)); } void NumberSkeletonTest::unexpectedTokens() { @@ -168,7 +171,7 @@ void NumberSkeletonTest::unexpectedTokens() { u"precision-integer/ group-off", u"precision-integer// group-off"}; - expectedErrorSkeleton(cases, sizeof(cases) / sizeof(*cases)); + expectedErrorSkeleton(cases, UPRV_LENGTHOF(cases)); } void NumberSkeletonTest::duplicateValues() { @@ -181,7 +184,7 @@ void NumberSkeletonTest::duplicateValues() { u"engineering compact-long", u"sign-auto sign-always"}; - expectedErrorSkeleton(cases, sizeof(cases) / sizeof(*cases)); + expectedErrorSkeleton(cases, UPRV_LENGTHOF(cases)); } void NumberSkeletonTest::stemsRequiringOption() { @@ -224,6 +227,7 @@ void NumberSkeletonTest::defaultTokens() { skeletonString, status).toSkeleton(status); // Skeleton should become empty when normalized assertEquals(skeletonString, u"", normalized); + status.errIfFailureAndReset(); } } @@ -246,6 +250,7 @@ void NumberSkeletonTest::flexibleSeparators() { .formatDouble(5142.3, status) .toString(); assertEquals(skeletonString, expected, actual); + status.errIfFailureAndReset(); } } diff --git a/icu4j/main/classes/core/src/com/ibm/icu/number/NumberSkeletonImpl.java b/icu4j/main/classes/core/src/com/ibm/icu/number/NumberSkeletonImpl.java index e93fd0db27e..8758a839ff6 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/number/NumberSkeletonImpl.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/number/NumberSkeletonImpl.java @@ -589,7 +589,7 @@ class NumberSkeletonImpl { } /** - * Given that the current segment represents an stem, parse it and save the result. + * Given that the current segment represents a stem, parse it and save the result. * * @return The next state after parsing this stem, corresponding to what subset of options to expect. */