Changeset 247286 in webkit


Ignore:
Timestamp:
Jul 9, 2019 4:54:07 PM (5 years ago)
Author:
weinig@apple.com
Message:

Add StringBuilder member function which allows makeString() style variadic argument construction
​https://bugs.webkit.org/show_bug.cgi?id=198997

Reviewed by Darin Adler.

Source/WTF:

Adds new StringBuilder::flexibleAppend(...) member function which allows passing one or more
string-adaptable (in the sense that there is StringTypeAdapter specialization for the
type) parameters. This re-ususes the variadic template infrastructure in StringConcatenate.h
that is used for makeString(...) allowing for improvements in one to benefit the other.

The advantage of StringBuilder::flexibleAppend(...) over calling StringBuilder::append(...)
multiple times (beyond the code sharing with makeString(...) is that it can avoid unnecessary
additional re-allocations when the StringBuilder needs to expand it's capacity. It does this
by computing the complete required length for all the passed arguments and then ensuring enough
capacity is available. It also reduces the allocation overhead versus the anti-pattern of
builder.append(makeString(...)).

Ideally, this member function should eventually just be called StringBuilder::append(...), but
the current overload set for StringBuilder::append(...) makes this complicated due to overloads
that take two arguments such as StringBuilder::append(const UChar*, unsigned). Going forward, we
should rename or remove those overloads and move to a standard interface.

  • wtf/posix/FileSystemPOSIX.cpp:

(WTF::FileSystemImpl::pathByAppendingComponents):
Adopt StringBuilder::flexibleAppend, using to combine the append of '/' and component.

  • wtf/text/StringBuilder.cpp:

(WTF::StringBuilder::appendUninitialized):
(WTF::StringBuilder::appendUninitializedWithoutOverflowCheck):
Extract the part of appendUnitialized that doesn't do the overflow check
into it's own member function to allow callers that have already done the
overflow check to bypass it.

(WTF::StringBuilder::appendUninitializedWithoutOverflowCheckForUChar):
(WTF::StringBuilder::appendUninitializedWithoutOverflowCheckForLChar):
Added to allow template member function flexibleAppendFromAdapters to call
appendUninitializedWithoutOverflowCheck without moving it to the header.

  • wtf/text/StringBuilder.h:

(WTF::StringBuilder::flexibleAppendFromAdapters):
(WTF::StringBuilder::flexibleAppend):
Modeled on tryMakeStringFromAdapters in StringConcatenate.h, these
eagerly compute the required length, expand the buffer and then use
the existing string type adaptor accumulation functions used by makeString.

  • wtf/text/StringConcatenate.h:

(WTF::stringTypeAdapterAccumulator):
(WTF::tryMakeStringFromAdapters):
(WTF::makeStringAccumulator): Deleted.
Renames makeStringAccumulator to stringTypeAdapterAccumulator now that it is used
by more than just makeString.

Tools:

  • TestWebKitAPI/Tests/WTF/StringBuilder.cpp:

Add basic test showing that StringBuilder::flexibleAppend can be used to
append one or more string adaptable types.

Location:
trunk
Files:
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WTF/ChangeLog

    r247269 r247286  
     12019-07-09  Sam Weinig  <weinig@apple.com>
     2
     3        Add StringBuilder member function which allows makeString() style variadic argument construction
     4        https://bugs.webkit.org/show_bug.cgi?id=198997
     5
     6        Reviewed by Darin Adler.
     7
     8        Adds new StringBuilder::flexibleAppend(...) member function which allows passing one or more
     9        string-adaptable (in the sense that there is StringTypeAdapter specialization for the
     10        type) parameters. This re-ususes the variadic template infrastructure in StringConcatenate.h
     11        that is used for makeString(...) allowing for improvements in one to benefit the other.
     12
     13        The advantage of StringBuilder::flexibleAppend(...) over calling StringBuilder::append(...)
     14        multiple times (beyond the code sharing with makeString(...) is that it can avoid unnecessary
     15        additional re-allocations when the StringBuilder needs to expand it's capacity. It does this
     16        by computing the complete required length for all the passed arguments and then ensuring enough
     17        capacity is available. It also reduces the allocation overhead versus the anti-pattern of
     18        builder.append(makeString(...)).
     19
     20        Ideally, this member function should eventually just be called StringBuilder::append(...), but
     21        the current overload set for StringBuilder::append(...) makes this complicated due to overloads
     22        that take two arguments such as StringBuilder::append(const UChar*, unsigned). Going forward, we
     23        should rename or remove those overloads and move to a standard interface.
     24
     25        * wtf/posix/FileSystemPOSIX.cpp:
     26        (WTF::FileSystemImpl::pathByAppendingComponents):
     27        Adopt StringBuilder::flexibleAppend, using to combine the append of '/' and component.
     28
     29        * wtf/text/StringBuilder.cpp:
     30        (WTF::StringBuilder::appendUninitialized):
     31        (WTF::StringBuilder::appendUninitializedWithoutOverflowCheck):
     32        Extract the part of appendUnitialized that doesn't do the overflow check
     33        into it's own member function to allow callers that have already done the
     34        overflow check to bypass it.
     35
     36        (WTF::StringBuilder::appendUninitializedWithoutOverflowCheckForUChar):
     37        (WTF::StringBuilder::appendUninitializedWithoutOverflowCheckForLChar):
     38        Added to allow template member function flexibleAppendFromAdapters to call
     39        appendUninitializedWithoutOverflowCheck without moving it to the header.
     40       
     41        * wtf/text/StringBuilder.h:
     42        (WTF::StringBuilder::flexibleAppendFromAdapters):
     43        (WTF::StringBuilder::flexibleAppend):
     44        Modeled on tryMakeStringFromAdapters in StringConcatenate.h, these
     45        eagerly compute the required length, expand the buffer and then use
     46        the existing string type adaptor accumulation functions used by makeString.
     47
     48        * wtf/text/StringConcatenate.h:
     49        (WTF::stringTypeAdapterAccumulator):
     50        (WTF::tryMakeStringFromAdapters):
     51        (WTF::makeStringAccumulator): Deleted.
     52        Renames makeStringAccumulator to stringTypeAdapterAccumulator now that it is used
     53        by more than just makeString.
     54
    1552019-07-09  Alex Christensen  <achristensen@webkit.org>
    256

  • trunk/Source/WTF/wtf/posix/FileSystemPOSIX.cpp

    r245186 r247286  
    302302    StringBuilder builder;
    303303    builder.append(path);
    304     for (auto& component : components) {
    305         builder.append('/');
    306         builder.append(component);
    307     }
     304    for (auto& component : components)
     305        builder.flexibleAppend('/', component);
    308306    return builder.toString();
    309307}

  • trunk/Source/WTF/wtf/text/StringBuilder.cpp

    r246034 r247286  
    164164}
    165165
    166 template <>
     166template<>
    167167void StringBuilder::reallocateBuffer<LChar>(unsigned requiredLength)
    168168{
    … …  
    184184}
    185185
    186 template <>
     186template<>
    187187void StringBuilder::reallocateBuffer<UChar>(unsigned requiredLength)
    188188{
    … …  
    232232}
    233233
    234 // Make 'length' additional capacity be available in m_buffer, update m_string & m_length,
     234// Make 'additionalLength' additional capacity be available in m_buffer, update m_string & m_length,
    235235// return a pointer to the newly allocated storage.
    236236// Returns nullptr if the size of the new builder would have overflowed
    237 template <typename CharType>
    238 ALWAYS_INLINE CharType* StringBuilder::appendUninitialized(unsigned length)
    239 {
    240     ASSERT(length);
     237template<typename CharacterType>
     238ALWAYS_INLINE CharacterType* StringBuilder::appendUninitialized(unsigned additionalLength)
     239{
     240    ASSERT(additionalLength);
    241241
    242242    // Calculate the new size of the builder after appending.
    243     CheckedInt32 requiredLength = m_length + length;
     243    CheckedInt32 requiredLength = m_length + additionalLength;
    244244    if (requiredLength.hasOverflowed()) {
    245245        didOverflow();
    246246        return nullptr;
    247247    }
     248
     249    return appendUninitializedWithoutOverflowCheck<CharacterType>(requiredLength);
     250}
     251
     252template<typename CharacterType>
     253ALWAYS_INLINE CharacterType* StringBuilder::appendUninitializedWithoutOverflowCheck(CheckedInt32 requiredLength)
     254{
     255    ASSERT(!requiredLength.hasOverflowed());
    248256
    249257    if (m_buffer && (requiredLength.unsafeGet<unsigned>() <= m_buffer->length())) {
    … …  
    253261        m_string = String();
    254262        m_length = requiredLength;
    255         return getBufferCharacters<CharType>() + currentLength;
    256     }
    257    
    258     return appendUninitializedSlow<CharType>(requiredLength.unsafeGet());
    259 }
    260 
    261 // Make 'length' additional capacity be available in m_buffer, update m_string & m_length,
     263        return getBufferCharacters<CharacterType>() + currentLength;
     264    }
     265   
     266    return appendUninitializedSlow<CharacterType>(requiredLength.unsafeGet());
     267}
     268
     269UChar* StringBuilder::appendUninitializedWithoutOverflowCheckForUChar(CheckedInt32 requiredLength)
     270{
     271    return appendUninitializedWithoutOverflowCheck<UChar>(requiredLength);
     272}
     273
     274LChar* StringBuilder::appendUninitializedWithoutOverflowCheckForLChar(CheckedInt32 requiredLength)
     275{
     276    return appendUninitializedWithoutOverflowCheck<LChar>(requiredLength);
     277}
     278
     279// Make 'requiredLength' capacity be available in m_buffer, update m_string & m_length,
    262280// return a pointer to the newly allocated storage.
    263 template <typename CharType>
    264 CharType* StringBuilder::appendUninitializedSlow(unsigned requiredLength)
     281template<typename CharacterType>
     282CharacterType* StringBuilder::appendUninitializedSlow(unsigned requiredLength)
    265283{
    266284    ASSERT(!hasOverflowed());
    … …  
    271289        ASSERT(m_buffer->length() >= m_length.unsafeGet<unsigned>());
    272290       
    273         reallocateBuffer<CharType>(expandedCapacity(capacity(), requiredLength));
     291        reallocateBuffer<CharacterType>(expandedCapacity(capacity(), requiredLength));
    274292    } else {
    275293        ASSERT(m_string.length() == m_length.unsafeGet<unsigned>());
    276         allocateBuffer(m_length ? m_string.characters<CharType>() : nullptr, expandedCapacity(capacity(), requiredLength));
     294        allocateBuffer(m_length ? m_string.characters<CharacterType>() : nullptr, expandedCapacity(capacity(), requiredLength));
    277295    }
    278296    if (UNLIKELY(hasOverflowed()))
    279297        return nullptr;
    280298
    281     CharType* result = getBufferCharacters<CharType>() + m_length.unsafeGet();
     299    CharacterType* result = getBufferCharacters<CharacterType>() + m_length.unsafeGet();
    282300    m_length = requiredLength;
    283301    ASSERT(!hasOverflowed());

  • trunk/Source/WTF/wtf/text/StringBuilder.h

    r246596 r247286  
    232232    WTF_EXPORT_PRIVATE void appendFixedWidthNumber(double, unsigned decimalPlaces);
    233233
     234    // FIXME: Rename to append(...) after renaming any overloads of append that take more than one argument.
     235    template<typename... StringTypes> void flexibleAppend(StringTypes...);
     236
    234237    String toString()
    235238    {
    … …  
    351354    void allocateBuffer(const UChar* currentCharacters, unsigned requiredLength);
    352355    void allocateBufferUpConvert(const LChar* currentCharacters, unsigned requiredLength);
    353     template <typename CharType>
    354     void reallocateBuffer(unsigned requiredLength);
    355     template <typename CharType>
    356     ALWAYS_INLINE CharType* appendUninitialized(unsigned length);
    357     template <typename CharType>
    358     CharType* appendUninitializedSlow(unsigned length);
    359     template <typename CharType>
    360     ALWAYS_INLINE CharType * getBufferCharacters();
     356    template<typename CharacterType> void reallocateBuffer(unsigned requiredLength);
     357    template<typename CharacterType> ALWAYS_INLINE CharacterType* appendUninitialized(unsigned additionalLength);
     358    template<typename CharacterType> ALWAYS_INLINE CharacterType* appendUninitializedWithoutOverflowCheck(CheckedInt32 requiredLength);
     359    template<typename CharacterType> CharacterType* appendUninitializedSlow(unsigned requiredLength);
     360   
     361    WTF_EXPORT_PRIVATE UChar* appendUninitializedWithoutOverflowCheckForUChar(CheckedInt32 requiredLength);
     362    WTF_EXPORT_PRIVATE LChar* appendUninitializedWithoutOverflowCheckForLChar(CheckedInt32 requiredLength);
     363   
     364    template<typename CharacterType> ALWAYS_INLINE CharacterType* getBufferCharacters();
    361365    WTF_EXPORT_PRIVATE void reifyString() const;
     366
     367    template<typename... StringTypeAdapters> void flexibleAppendFromAdapters(StringTypeAdapters...);
    362368
    363369    mutable String m_string;
    … …  
    375381};
    376382
    377 template <>
     383template<>
    378384ALWAYS_INLINE LChar* StringBuilder::getBufferCharacters<LChar>()
    379385{
    … …  
    382388}
    383389
    384 template <>
     390template<>
    385391ALWAYS_INLINE UChar* StringBuilder::getBufferCharacters<UChar>()
    386392{
    … …  
    389395}
    390396
    391 template <typename CharType>
    392 bool equal(const StringBuilder& s, const CharType* buffer, unsigned length)
     397template<typename... StringTypeAdapters>
     398void StringBuilder::flexibleAppendFromAdapters(StringTypeAdapters... adapters)
     399{
     400    auto requiredLength = checkedSum<int32_t>(m_length, adapters.length()...);
     401    if (requiredLength.hasOverflowed()) {
     402        didOverflow();
     403        return;
     404    }
     405
     406    if (m_is8Bit && are8Bit(adapters...)) {
     407        LChar* dest = appendUninitializedWithoutOverflowCheckForLChar(requiredLength);
     408        if (!dest) {
     409            ASSERT(hasOverflowed());
     410            return;
     411        }
     412        stringTypeAdapterAccumulator(dest, adapters...);
     413    } else {
     414        UChar* dest = appendUninitializedWithoutOverflowCheckForUChar(requiredLength);
     415        if (!dest) {
     416            ASSERT(hasOverflowed());
     417            return;
     418        }
     419        stringTypeAdapterAccumulator(dest, adapters...);
     420    }
     421}
     422
     423template<typename... StringTypes>
     424void StringBuilder::flexibleAppend(StringTypes... strings)
     425{
     426    flexibleAppendFromAdapters(StringTypeAdapter<StringTypes>(strings)...);
     427}
     428
     429template<typename CharacterType>
     430bool equal(const StringBuilder& s, const CharacterType* buffer, unsigned length)
    393431{
    394432    if (s.length() != length)
    … …  
    401439}
    402440
    403 template <typename StringType>
     441template<typename StringType>
    404442bool equal(const StringBuilder& a, const StringType& b)
    405443{

  • trunk/Source/WTF/wtf/text/StringConcatenate.h

    r246490 r247286  
    249249
    250250template<typename ResultType, typename Adapter>
    251 inline void makeStringAccumulator(ResultType* result, Adapter adapter)
     251inline void stringTypeAdapterAccumulator(ResultType* result, Adapter adapter)
    252252{
    253253    adapter.writeTo(result);
    … …  
    255255
    256256template<typename ResultType, typename Adapter, typename... Adapters>
    257 inline void makeStringAccumulator(ResultType* result, Adapter adapter, Adapters ...adapters)
     257inline void stringTypeAdapterAccumulator(ResultType* result, Adapter adapter, Adapters ...adapters)
    258258{
    259259    adapter.writeTo(result);
    260     makeStringAccumulator(result + adapter.length(), adapters...);
     260    stringTypeAdapterAccumulator(result + adapter.length(), adapters...);
    261261}
    262262
    … …  
    277277            return String();
    278278
    279         makeStringAccumulator(buffer, adapter, adapters...);
     279        stringTypeAdapterAccumulator(buffer, adapter, adapters...);
    280280
    281281        return resultImpl;
    … …  
    287287        return String();
    288288
    289     makeStringAccumulator(buffer, adapter, adapters...);
     289    stringTypeAdapterAccumulator(buffer, adapter, adapters...);
    290290
    291291    return resultImpl;

  • trunk/Tools/ChangeLog

    r247270 r247286  
     12019-07-09  Sam Weinig  <weinig@apple.com>
     2
     3        Add StringBuilder member function which allows makeString() style variadic argument construction
     4        https://bugs.webkit.org/show_bug.cgi?id=198997
     5
     6        Reviewed by Darin Adler.
     7
     8        * TestWebKitAPI/Tests/WTF/StringBuilder.cpp:
     9        Add basic test showing that StringBuilder::flexibleAppend can be used to
     10        append one or more string adaptable types.
     11
    1122019-07-09  Sihui Liu  <sihui_liu@apple.com>
    213

  • trunk/Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp

    r246490 r247286  
    117117}
    118118
     119TEST(StringBuilderTest, FlexibleAppend)
     120{
     121    {
     122        StringBuilder builder;
     123        builder.flexibleAppend(String("0123456789"));
     124        expectBuilderContent("0123456789", builder);
     125        builder.flexibleAppend("abcd");
     126        expectBuilderContent("0123456789abcd", builder);
     127        builder.flexibleAppend('e');
     128        expectBuilderContent("0123456789abcde", builder);
     129        builder.flexibleAppend("");
     130        expectBuilderContent("0123456789abcde", builder);
     131    }
     132
     133    {
     134        StringBuilder builder;
     135        builder.flexibleAppend(String("0123456789"), "abcd", 'e', "");
     136        expectBuilderContent("0123456789abcde", builder);
     137        builder.flexibleAppend(String("A"), "B", 'C', "");
     138        expectBuilderContent("0123456789abcdeABC", builder);
     139    }
     140}
     141
    119142TEST(StringBuilderTest, ToString)
    120143{
Note: See TracChangeset for help on using the changeset viewer.