Reply to code review feedback

diff --git a/icu4c/source/common/cmemory.h b/icu4c/source/common/cmemory.h
index f3ba54d2ed6770ae7355807b4725f3f53319941b..8d6044202155040b9faf90e55429cbfa46c0a03e 100644 (file)
--- a/icu4c/source/common/cmemory.h
+++ b/icu4c/source/common/cmemory.h
@@ -274,7 +274,10 @@ inline T *LocalMemory<T>::allocateInsteadAndCopy(int32_t newCapacity, int32_t le
  *
  * WARNING: MaybeStackArray only works with primitive (plain-old data) types.
  * It does NOT know how to call a destructor! If you work with classes with
- * destructors, consider LocalArray in localpointer.h or MemoryPool.
+ * destructors, consider:
+ *
+ * - LocalArray in localpointer.h if you know the length ahead of time
+ * - MaybeStackVector if you know the length at runtime
  */
 template<typename T, int32_t stackCapacity>
 class MaybeStackArray {
@@ -740,6 +743,8 @@ protected:
 /**
  * An internal Vector-like implementation based on MemoryPool.
  *
+ * Heap-allocates each element and stores pointers.
+ *
  * To append an item to the vector, use emplaceBack.
  *
  *     MaybeStackVector<MyType> vector;
@@ -774,13 +779,23 @@ public:
         return this->fPool.getAlias();
     }
 
+    /**
+     * Array item access (read-only).
+     * No index bounds check.
+     * @param i array index
+     * @return reference to the array item
+     */
+    const T* operator[](ptrdiff_t i) const {
+        return this->fPool[i];
+    }
+
     /**
      * Array item access (writable).
      * No index bounds check.
      * @param i array index
      * @return reference to the array item
      */
-    T* operator[](ptrdiff_t i) const {
+    T* operator[](ptrdiff_t i) {
         return this->fPool[i];
     }
 

diff --git a/icu4c/source/i18n/currunit.cpp b/icu4c/source/i18n/currunit.cpp
index dab6dda536826b4a3ea0ef206c2c420d289712c5..280bd563e5b109a708f12969d8c27d8b5efc939c 100644 (file)
--- a/icu4c/source/i18n/currunit.cpp
+++ b/icu4c/source/i18n/currunit.cpp
@@ -19,9 +19,7 @@
 #include "cstring.h"
 #include "uinvchar.h"
 #include "charstr.h"
-
-static constexpr char16_t kDefaultCurrency[] = u"XXX";
-static constexpr char kDefaultCurrency8[] = "XXX";
+#include "measunit_impl.h"
 
 U_NAMESPACE_BEGIN
 

diff --git a/icu4c/source/i18n/measunit.cpp b/icu4c/source/i18n/measunit.cpp
index 798f8e549d1b12caf811fc4e1929fd532e6ac255..efc55d028666eeee720059bfbfcccbcad0672899 100644 (file)
--- a/icu4c/source/i18n/measunit.cpp
+++ b/icu4c/source/i18n/measunit.cpp
@@ -2273,7 +2273,7 @@ void MeasureUnit::initCurrency(StringPiece isoCurrency) {
         }
         // malloc error: fall back to the undefined currency
         result = binarySearch(
-            gSubTypes, gOffsets[fTypeId], gOffsets[fTypeId + 1], "XXX");
+            gSubTypes, gOffsets[fTypeId], gOffsets[fTypeId + 1], kDefaultCurrency8);
         U_ASSERT(result != -1);
     }
     fSubTypeId = result - gOffsets[fTypeId];

diff --git a/icu4c/source/i18n/measunit_extra.cpp b/icu4c/source/i18n/measunit_extra.cpp
index 6d23f4935c7ee67eca9df55907161595dcbf6b82..5a61b00914b45a8eb67048895acf719897bfe097 100644 (file)
--- a/icu4c/source/i18n/measunit_extra.cpp
+++ b/icu4c/source/i18n/measunit_extra.cpp
@@ -276,17 +276,20 @@ public:
     Type getType() const {
         if (fMatch <= 0) {
             UPRV_UNREACHABLE;
-        } else if (fMatch < kCompoundPartOffset) {
+        }
+        if (fMatch < kCompoundPartOffset) {
             return TYPE_SI_PREFIX;
-        } else if (fMatch < kPowerPartOffset) {
+        }
+        if (fMatch < kPowerPartOffset) {
             return TYPE_COMPOUND_PART;
-        } else if (fMatch < kSimpleUnitOffset) {
+        }
+        if (fMatch < kSimpleUnitOffset) {
             return TYPE_POWER_PART;
-        } else if (fMatch == kSimpleUnitOffset) {
+        }
+        if (fMatch == kSimpleUnitOffset) {
             return TYPE_ONE;
-        } else {
-            return TYPE_SIMPLE_UNIT;
         }
+        return TYPE_SIMPLE_UNIT;
     }
 
     UMeasureSIPrefix getSIPrefix() const {
@@ -652,15 +655,15 @@ SingleUnitImpl SingleUnitImpl::forMeasureUnit(const MeasureUnit& measureUnit, UE
     }
     if (impl.units.length() == 0) {
         return {};
-    } else if (impl.units.length() == 1) {
+    }
+    if (impl.units.length() == 1) {
         return *impl.units[0];
-    } else {
-        status = U_ILLEGAL_ARGUMENT_ERROR;
-        return {};
     }
+    status = U_ILLEGAL_ARGUMENT_ERROR;
+    return {};
 }
 
-MeasureUnit SingleUnitImpl::build(UErrorCode& status) {
+MeasureUnit SingleUnitImpl::build(UErrorCode& status) const {
     MeasureUnitImpl temp;
     temp.append(*this, status);
     return std::move(temp).build(status);

diff --git a/icu4c/source/i18n/measunit_impl.h b/icu4c/source/i18n/measunit_impl.h
index 57784c53d7baae1febf3a265887ad094dd2e3826..5d2e6b1fe57aec3833247bff4464a47ca502a654 100644 (file)
--- a/icu4c/source/i18n/measunit_impl.h
+++ b/icu4c/source/i18n/measunit_impl.h
@@ -15,6 +15,10 @@
 U_NAMESPACE_BEGIN
 
 
+static const char16_t kDefaultCurrency[] = u"XXX";
+static const char kDefaultCurrency8[] = "XXX";
+
+
 /**
  * A struct representing a single unit (optional SI prefix and dimensionality).
  */
@@ -26,26 +30,30 @@ struct SingleUnitImpl : public UMemory {
     static SingleUnitImpl forMeasureUnit(const MeasureUnit& measureUnit, UErrorCode& status);
 
     /** Transform this SingleUnitImpl into a MeasureUnit, simplifying if possible. */
-    MeasureUnit build(UErrorCode& status);
+    MeasureUnit build(UErrorCode& status) const;
 
     /** Compare this SingleUnitImpl to another SingleUnitImpl. */
     int32_t compareTo(const SingleUnitImpl& other) const {
         if (dimensionality < 0 && other.dimensionality > 0) {
             // Positive dimensions first
             return 1;
-        } else if (dimensionality > 0 && other.dimensionality < 0) {
+        }
+        if (dimensionality > 0 && other.dimensionality < 0) {
             return -1;
-        } else if (index < other.index) {
+        }
+        if (index < other.index) {
             return -1;
-        } else if (index > other.index) {
+        }
+        if (index > other.index) {
             return 1;
-        } else if (siPrefix < other.siPrefix) {
+        }
+        if (siPrefix < other.siPrefix) {
             return -1;
-        } else if (siPrefix > other.siPrefix) {
+        }
+        if (siPrefix > other.siPrefix) {
             return 1;
-        } else {
-            return 0;
         }
+        return 0;
     }
 
     /**