From f39e5259a67b740bf08a18fc54502076a79a1ece Mon Sep 17 00:00:00 2001 From: "Shane F. Carr" Date: Mon, 23 Mar 2020 13:06:09 -0500 Subject: [PATCH] Reply to code review feedback --- icu4c/source/common/cmemory.h | 19 +++++++++++++++++-- icu4c/source/i18n/currunit.cpp | 4 +--- icu4c/source/i18n/measunit.cpp | 2 +- icu4c/source/i18n/measunit_extra.cpp | 25 ++++++++++++++----------- icu4c/source/i18n/measunit_impl.h | 24 ++++++++++++++++-------- 5 files changed, 49 insertions(+), 25 deletions(-) diff --git a/icu4c/source/common/cmemory.h b/icu4c/source/common/cmemory.h index f3ba54d2ed6..8d604420215 100644 --- a/icu4c/source/common/cmemory.h +++ b/icu4c/source/common/cmemory.h @@ -274,7 +274,10 @@ inline T *LocalMemory::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 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 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 dab6dda5368..280bd563e5b 100644 --- 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 798f8e549d1..efc55d02866 100644 --- 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 6d23f4935c7..5a61b00914b 100644 --- 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 57784c53d7b..5d2e6b1fe57 100644 --- 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; } /** -- 2.50.1