From: Shane F. Carr Date: Mon, 23 Mar 2020 22:51:32 +0000 (-0500) Subject: Reply to code review feedback X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=787c3cc2a82d904579fe755b122f188b5ebedc79;p=icu Reply to code review feedback --- diff --git a/icu4c/source/i18n/measunit.cpp b/icu4c/source/i18n/measunit.cpp index efc55d02866..211e3894eed 100644 --- a/icu4c/source/i18n/measunit.cpp +++ b/icu4c/source/i18n/measunit.cpp @@ -2038,7 +2038,7 @@ MeasureUnit &MeasureUnit::operator=(const MeasureUnit &other) { if (this == &other) { return *this; } - uprv_free(fImpl); + delete fImpl; if (other.fImpl) { ErrorCode localStatus; fImpl = new MeasureUnitImpl(other.fImpl->copy(localStatus)); @@ -2059,7 +2059,7 @@ MeasureUnit &MeasureUnit::operator=(MeasureUnit &&other) noexcept { if (this == &other) { return *this; } - uprv_free(fImpl); + delete fImpl; fImpl = other.fImpl; other.fImpl = nullptr; fTypeId = other.fTypeId; @@ -2291,7 +2291,7 @@ void MeasureUnit::initNoUnit(const char *subtype) { void MeasureUnit::setTo(int32_t typeId, int32_t subTypeId) { fTypeId = typeId; fSubTypeId = subTypeId; - uprv_free(fImpl); + delete fImpl; fImpl = nullptr; } diff --git a/icu4c/source/i18n/measunit_extra.cpp b/icu4c/source/i18n/measunit_extra.cpp index 5a61b00914b..3af1b9fcbf0 100644 --- a/icu4c/source/i18n/measunit_extra.cpp +++ b/icu4c/source/i18n/measunit_extra.cpp @@ -31,6 +31,9 @@ U_NAMESPACE_BEGIN namespace { +// TODO: Propose a new error code for this? +constexpr UErrorCode kUnitIdentifierSyntaxError = U_ILLEGAL_ARGUMENT_ERROR; + // This is to ensure we only insert positive integers into the trie constexpr int32_t kSIPrefixOffset = 64; @@ -356,27 +359,25 @@ private: int32_t match = -1; int32_t previ = -1; do { - fTrie.next(fSource.data()[fIndex++]); - if (fTrie.current() == USTRINGTRIE_NO_MATCH) { + auto result = fTrie.next(fSource.data()[fIndex++]); + if (result == USTRINGTRIE_NO_MATCH) { break; - } else if (fTrie.current() == USTRINGTRIE_NO_VALUE) { + } else if (result == USTRINGTRIE_NO_VALUE) { continue; - } else if (fTrie.current() == USTRINGTRIE_FINAL_VALUE) { - match = fTrie.getValue(); - previ = fIndex; + } + U_ASSERT(USTRINGTRIE_HAS_VALUE(result)); + match = fTrie.getValue(); + previ = fIndex; + if (result == USTRINGTRIE_FINAL_VALUE) { break; - } else if (fTrie.current() == USTRINGTRIE_INTERMEDIATE_VALUE) { - match = fTrie.getValue(); - previ = fIndex; + } else if (result == USTRINGTRIE_INTERMEDIATE_VALUE) { continue; - } else { - UPRV_UNREACHABLE; } + UPRV_UNREACHABLE; } while (fIndex < fSource.length()); if (match < 0) { - // TODO: Make a new status code? - status = U_ILLEGAL_ARGUMENT_ERROR; + status = kUnitIdentifierSyntaxError; } else { fIndex = previ; } @@ -408,12 +409,14 @@ private: return; } if (token.getType() != Token::TYPE_COMPOUND_PART) { - goto fail; + status = kUnitIdentifierSyntaxError; + return; } switch (token.getMatch()) { case COMPOUND_PART_PER: if (fAfterPer) { - goto fail; + status = kUnitIdentifierSyntaxError; + return; } fAfterPer = true; result.dimensionality = -1; @@ -440,7 +443,8 @@ private: switch (token.getType()) { case Token::TYPE_POWER_PART: if (state > 0) { - goto fail; + status = kUnitIdentifierSyntaxError; + return; } result.dimensionality *= token.getPower(); previ = fIndex; @@ -449,7 +453,8 @@ private: case Token::TYPE_SI_PREFIX: if (state > 1) { - goto fail; + status = kUnitIdentifierSyntaxError; + return; } result.siPrefix = token.getSIPrefix(); previ = fIndex; @@ -466,14 +471,13 @@ private: return; default: - goto fail; + status = kUnitIdentifierSyntaxError; + return; } } - fail: - // TODO: Make a new status code? - status = U_ILLEGAL_ARGUMENT_ERROR; - return; + // We ran out of tokens before finding a complete single unit. + status = kUnitIdentifierSyntaxError; } void parseImpl(MeasureUnitImpl& result, UErrorCode& status) { @@ -494,7 +498,7 @@ private: bool added = result.append(singleUnit, status); if (sawPlus && !added) { // Two similar units are not allowed in a sequence unit - status = U_ILLEGAL_ARGUMENT_ERROR; + status = kUnitIdentifierSyntaxError; return; } if ((++unitNum) >= 2) { @@ -506,7 +510,7 @@ private: result.complexity = complexity; } else if (result.complexity != complexity) { // Mixed sequence and compound units - status = U_ILLEGAL_ARGUMENT_ERROR; + status = kUnitIdentifierSyntaxError; return; } } @@ -552,7 +556,7 @@ void serializeSingle(const SingleUnitImpl& singleUnit, bool first, CharString& o output.append('0' + (posPower % 10), status); output.append('-', status); } else { - status = U_ILLEGAL_ARGUMENT_ERROR; + status = kUnitIdentifierSyntaxError; } if (U_FAILURE(status)) { return; diff --git a/icu4c/source/i18n/unicode/measunit.h b/icu4c/source/i18n/unicode/measunit.h index 2a00d537d66..97d26ff45f2 100644 --- a/icu4c/source/i18n/unicode/measunit.h +++ b/icu4c/source/i18n/unicode/measunit.h @@ -249,14 +249,14 @@ class U_I18N_API MeasureUnit: public UObject { * @stable ICU 3.0 */ MeasureUnit(const MeasureUnit &other); - + +#ifndef U_HIDE_DRAFT_API /** * Move constructor. - * @stable ICU 3.0 + * @draft ICU 67 */ MeasureUnit(MeasureUnit &&other) noexcept; -#ifndef U_HIDE_DRAFT_API /** * Construct a MeasureUnit from a CLDR Sequence Unit Identifier, defined in UTS 35. * Validates and canonicalizes the identifier. @@ -278,11 +278,13 @@ class U_I18N_API MeasureUnit: public UObject { */ MeasureUnit &operator=(const MeasureUnit &other); +#ifndef U_HIDE_DRAFT_API /** * Move assignment operator. - * @stable ICU 3.0 + * @draft ICU 67 */ MeasureUnit &operator=(MeasureUnit &&other) noexcept; +#endif // U_HIDE_DRAFT_API /** * Returns a polymorphic clone of this object. The result will diff --git a/icu4c/source/test/intltest/measfmttest.cpp b/icu4c/source/test/intltest/measfmttest.cpp index 012f9a64a43..4846540dbdb 100644 --- a/icu4c/source/test/intltest/measfmttest.cpp +++ b/icu4c/source/test/intltest/measfmttest.cpp @@ -3260,6 +3260,7 @@ void MeasureFormatTest::TestInvalidIdentifiers() { }; for (const auto& input : inputs) { + status.setScope(input); MeasureUnit::forIdentifier(input, status); status.expectErrorAndReset(U_ILLEGAL_ARGUMENT_ERROR); }