Reply to code review feedback

diff --git a/icu4c/source/i18n/measunit.cpp b/icu4c/source/i18n/measunit.cpp
index efc55d028666eeee720059bfbfcccbcad0672899..211e3894eed2d95083f1a280c4b4e4078c38630d 100644 (file)
--- 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 5a61b00914b45a8eb67048895acf719897bfe097..3af1b9fcbf0db533a538d2324f9a45973f3eb4f4 100644 (file)
--- 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 2a00d537d668ad5e641e7ae1b8da17115507e08f..97d26ff45f2e663e373ddb99011121ef275a199d 100644 (file)
--- 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 012f9a64a43b4707386daa551cc9bc564a3fb8c1..4846540dbdbe36f800e03389fbe72b2be13e4029 100644 (file)
--- 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);
     }