Add StringBuilder member function which allows makeString() style variadic argument...
authorweinig@apple.com <weinig@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 9 Jul 2019 23:54:07 +0000 (23:54 +0000)
committerweinig@apple.com <weinig@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 9 Jul 2019 23:54:07 +0000 (23:54 +0000)
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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@247286 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WTF/ChangeLog
Source/WTF/wtf/posix/FileSystemPOSIX.cpp
Source/WTF/wtf/text/StringBuilder.cpp
Source/WTF/wtf/text/StringBuilder.h
Source/WTF/wtf/text/StringConcatenate.h
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp

index e1f6502..c484fa1 100644 (file)
@@ -1,3 +1,57 @@
+2019-07-09  Sam Weinig  <weinig@apple.com>
+
+        Add StringBuilder member function which allows makeString() style variadic argument construction
+        https://bugs.webkit.org/show_bug.cgi?id=198997
+
+        Reviewed by Darin Adler.
+
+        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.
+
 2019-07-09  Alex Christensen  <achristensen@webkit.org>
 
         When parsing an IPv4 address, wait until after deciding it is indeed an IPv4 address before reporting syntax violations
index ad3c926..c030f9c 100644 (file)
@@ -301,10 +301,8 @@ String pathByAppendingComponents(StringView path, const Vector<StringView>& comp
 {
     StringBuilder builder;
     builder.append(path);
-    for (auto& component : components) {
-        builder.append('/');
-        builder.append(component);
-    }
+    for (auto& component : components)
+        builder.flexibleAppend('/', component);
     return builder.toString();
 }
 
index da1d4a4..89e16ef 100644 (file)
@@ -163,7 +163,7 @@ void StringBuilder::allocateBufferUpConvert(const LChar* currentCharacters, unsi
     ASSERT(m_buffer->length() == requiredLength);
 }
 
-template <>
+template<>
 void StringBuilder::reallocateBuffer<LChar>(unsigned requiredLength)
 {
     // If the buffer has only one ref (by this StringBuilder), reallocate it,
@@ -183,7 +183,7 @@ void StringBuilder::reallocateBuffer<LChar>(unsigned requiredLength)
     ASSERT(hasOverflowed() || m_buffer->length() == requiredLength);
 }
 
-template <>
+template<>
 void StringBuilder::reallocateBuffer<UChar>(unsigned requiredLength)
 {
     // If the buffer has only one ref (by this StringBuilder), reallocate it,
@@ -231,37 +231,55 @@ void StringBuilder::reserveCapacity(unsigned newCapacity)
     ASSERT(hasOverflowed() || !newCapacity || m_buffer->length() >= newCapacity);
 }
 
-// Make 'length' additional capacity be available in m_buffer, update m_string & m_length,
+// Make 'additionalLength' additional capacity be available in m_buffer, update m_string & m_length,
 // return a pointer to the newly allocated storage.
 // Returns nullptr if the size of the new builder would have overflowed
-template <typename CharType>
-ALWAYS_INLINE CharType* StringBuilder::appendUninitialized(unsigned length)
+template<typename CharacterType>
+ALWAYS_INLINE CharacterType* StringBuilder::appendUninitialized(unsigned additionalLength)
 {
-    ASSERT(length);
+    ASSERT(additionalLength);
 
     // Calculate the new size of the builder after appending.
-    CheckedInt32 requiredLength = m_length + length;
+    CheckedInt32 requiredLength = m_length + additionalLength;
     if (requiredLength.hasOverflowed()) {
         didOverflow();
         return nullptr;
     }
 
+    return appendUninitializedWithoutOverflowCheck<CharacterType>(requiredLength);
+}
+
+template<typename CharacterType>
+ALWAYS_INLINE CharacterType* StringBuilder::appendUninitializedWithoutOverflowCheck(CheckedInt32 requiredLength)
+{
+    ASSERT(!requiredLength.hasOverflowed());
+
     if (m_buffer && (requiredLength.unsafeGet<unsigned>() <= m_buffer->length())) {
         // If the buffer is valid it must be at least as long as the current builder contents!
         ASSERT(m_buffer->length() >= m_length.unsafeGet<unsigned>());
         unsigned currentLength = m_length.unsafeGet();
         m_string = String();
         m_length = requiredLength;
-        return getBufferCharacters<CharType>() + currentLength;
+        return getBufferCharacters<CharacterType>() + currentLength;
     }
     
-    return appendUninitializedSlow<CharType>(requiredLength.unsafeGet());
+    return appendUninitializedSlow<CharacterType>(requiredLength.unsafeGet());
+}
+
+UChar* StringBuilder::appendUninitializedWithoutOverflowCheckForUChar(CheckedInt32 requiredLength)
+{
+    return appendUninitializedWithoutOverflowCheck<UChar>(requiredLength);
+}
+
+LChar* StringBuilder::appendUninitializedWithoutOverflowCheckForLChar(CheckedInt32 requiredLength)
+{
+    return appendUninitializedWithoutOverflowCheck<LChar>(requiredLength);
 }
 
-// Make 'length' additional capacity be available in m_buffer, update m_string & m_length,
+// Make 'requiredLength' capacity be available in m_buffer, update m_string & m_length,
 // return a pointer to the newly allocated storage.
-template <typename CharType>
-CharType* StringBuilder::appendUninitializedSlow(unsigned requiredLength)
+template<typename CharacterType>
+CharacterType* StringBuilder::appendUninitializedSlow(unsigned requiredLength)
 {
     ASSERT(!hasOverflowed());
     ASSERT(requiredLength);
@@ -270,15 +288,15 @@ CharType* StringBuilder::appendUninitializedSlow(unsigned requiredLength)
         // If the buffer is valid it must be at least as long as the current builder contents!
         ASSERT(m_buffer->length() >= m_length.unsafeGet<unsigned>());
         
-        reallocateBuffer<CharType>(expandedCapacity(capacity(), requiredLength));
+        reallocateBuffer<CharacterType>(expandedCapacity(capacity(), requiredLength));
     } else {
         ASSERT(m_string.length() == m_length.unsafeGet<unsigned>());
-        allocateBuffer(m_length ? m_string.characters<CharType>() : nullptr, expandedCapacity(capacity(), requiredLength));
+        allocateBuffer(m_length ? m_string.characters<CharacterType>() : nullptr, expandedCapacity(capacity(), requiredLength));
     }
     if (UNLIKELY(hasOverflowed()))
         return nullptr;
 
-    CharType* result = getBufferCharacters<CharType>() + m_length.unsafeGet();
+    CharacterType* result = getBufferCharacters<CharacterType>() + m_length.unsafeGet();
     m_length = requiredLength;
     ASSERT(!hasOverflowed());
     ASSERT(m_buffer->length() >= m_length.unsafeGet<unsigned>());
index 998b857..18b66de 100644 (file)
@@ -231,6 +231,9 @@ public:
     WTF_EXPORT_PRIVATE void appendFixedWidthNumber(float, unsigned decimalPlaces);
     WTF_EXPORT_PRIVATE void appendFixedWidthNumber(double, unsigned decimalPlaces);
 
+    // FIXME: Rename to append(...) after renaming any overloads of append that take more than one argument.
+    template<typename... StringTypes> void flexibleAppend(StringTypes...);
+
     String toString()
     {
         if (!m_string.isNull()) {
@@ -350,16 +353,19 @@ private:
     void allocateBuffer(const LChar* currentCharacters, unsigned requiredLength);
     void allocateBuffer(const UChar* currentCharacters, unsigned requiredLength);
     void allocateBufferUpConvert(const LChar* currentCharacters, unsigned requiredLength);
-    template <typename CharType>
-    void reallocateBuffer(unsigned requiredLength);
-    template <typename CharType>
-    ALWAYS_INLINE CharType* appendUninitialized(unsigned length);
-    template <typename CharType>
-    CharType* appendUninitializedSlow(unsigned length);
-    template <typename CharType>
-    ALWAYS_INLINE CharType * getBufferCharacters();
+    template<typename CharacterType> void reallocateBuffer(unsigned requiredLength);
+    template<typename CharacterType> ALWAYS_INLINE CharacterType* appendUninitialized(unsigned additionalLength);
+    template<typename CharacterType> ALWAYS_INLINE CharacterType* appendUninitializedWithoutOverflowCheck(CheckedInt32 requiredLength);
+    template<typename CharacterType> CharacterType* appendUninitializedSlow(unsigned requiredLength);
+    
+    WTF_EXPORT_PRIVATE UChar* appendUninitializedWithoutOverflowCheckForUChar(CheckedInt32 requiredLength);
+    WTF_EXPORT_PRIVATE LChar* appendUninitializedWithoutOverflowCheckForLChar(CheckedInt32 requiredLength);
+    
+    template<typename CharacterType> ALWAYS_INLINE CharacterType* getBufferCharacters();
     WTF_EXPORT_PRIVATE void reifyString() const;
 
+    template<typename... StringTypeAdapters> void flexibleAppendFromAdapters(StringTypeAdapters...);
+
     mutable String m_string;
     RefPtr<StringImpl> m_buffer;
     union {
@@ -374,22 +380,54 @@ private:
 #endif
 };
 
-template <>
+template<>
 ALWAYS_INLINE LChar* StringBuilder::getBufferCharacters<LChar>()
 {
     ASSERT(m_is8Bit);
     return m_bufferCharacters8;
 }
 
-template <>
+template<>
 ALWAYS_INLINE UChar* StringBuilder::getBufferCharacters<UChar>()
 {
     ASSERT(!m_is8Bit);
     return m_bufferCharacters16;
 }
 
-template <typename CharType>
-bool equal(const StringBuilder& s, const CharType* buffer, unsigned length)
+template<typename... StringTypeAdapters>
+void StringBuilder::flexibleAppendFromAdapters(StringTypeAdapters... adapters)
+{
+    auto requiredLength = checkedSum<int32_t>(m_length, adapters.length()...);
+    if (requiredLength.hasOverflowed()) {
+        didOverflow();
+        return;
+    }
+
+    if (m_is8Bit && are8Bit(adapters...)) {
+        LChar* dest = appendUninitializedWithoutOverflowCheckForLChar(requiredLength);
+        if (!dest) {
+            ASSERT(hasOverflowed());
+            return;
+        }
+        stringTypeAdapterAccumulator(dest, adapters...);
+    } else {
+        UChar* dest = appendUninitializedWithoutOverflowCheckForUChar(requiredLength);
+        if (!dest) {
+            ASSERT(hasOverflowed());
+            return;
+        }
+        stringTypeAdapterAccumulator(dest, adapters...);
+    }
+}
+
+template<typename... StringTypes>
+void StringBuilder::flexibleAppend(StringTypes... strings)
+{
+    flexibleAppendFromAdapters(StringTypeAdapter<StringTypes>(strings)...);
+}
+
+template<typename CharacterType>
+bool equal(const StringBuilder& s, const CharacterType* buffer, unsigned length)
 {
     if (s.length() != length)
         return false;
@@ -400,7 +438,7 @@ bool equal(const StringBuilder& s, const CharType* buffer, unsigned length)
     return equal(s.characters16(), buffer, length);
 }
 
-template <typename StringType>
+template<typename StringType>
 bool equal(const StringBuilder& a, const StringType& b)
 {
     if (a.length() != b.length())
index c8cba07..0537b10 100644 (file)
@@ -248,16 +248,16 @@ inline bool are8Bit(Adapter adapter, Adapters ...adapters)
 }
 
 template<typename ResultType, typename Adapter>
-inline void makeStringAccumulator(ResultType* result, Adapter adapter)
+inline void stringTypeAdapterAccumulator(ResultType* result, Adapter adapter)
 {
     adapter.writeTo(result);
 }
 
 template<typename ResultType, typename Adapter, typename... Adapters>
-inline void makeStringAccumulator(ResultType* result, Adapter adapter, Adapters ...adapters)
+inline void stringTypeAdapterAccumulator(ResultType* result, Adapter adapter, Adapters ...adapters)
 {
     adapter.writeTo(result);
-    makeStringAccumulator(result + adapter.length(), adapters...);
+    stringTypeAdapterAccumulator(result + adapter.length(), adapters...);
 }
 
 template<typename StringTypeAdapter, typename... StringTypeAdapters>
@@ -276,7 +276,7 @@ String tryMakeStringFromAdapters(StringTypeAdapter adapter, StringTypeAdapters .
         if (!resultImpl)
             return String();
 
-        makeStringAccumulator(buffer, adapter, adapters...);
+        stringTypeAdapterAccumulator(buffer, adapter, adapters...);
 
         return resultImpl;
     }
@@ -286,7 +286,7 @@ String tryMakeStringFromAdapters(StringTypeAdapter adapter, StringTypeAdapters .
     if (!resultImpl)
         return String();
 
-    makeStringAccumulator(buffer, adapter, adapters...);
+    stringTypeAdapterAccumulator(buffer, adapter, adapters...);
 
     return resultImpl;
 }
index 2525ecd..06c94fb 100644 (file)
@@ -1,3 +1,14 @@
+2019-07-09  Sam Weinig  <weinig@apple.com>
+
+        Add StringBuilder member function which allows makeString() style variadic argument construction
+        https://bugs.webkit.org/show_bug.cgi?id=198997
+
+        Reviewed by Darin Adler.
+
+        * TestWebKitAPI/Tests/WTF/StringBuilder.cpp:
+        Add basic test showing that StringBuilder::flexibleAppend can be used to 
+        append one or more string adaptable types. 
+
 2019-07-09  Sihui Liu  <sihui_liu@apple.com>
 
         Only allow fetching and removing session credentials from WebsiteDataStore
index ab67ce1..e4b7c02 100644 (file)
@@ -116,6 +116,29 @@ TEST(StringBuilderTest, Append)
     }
 }
 
+TEST(StringBuilderTest, FlexibleAppend)
+{
+    {
+        StringBuilder builder;
+        builder.flexibleAppend(String("0123456789"));
+        expectBuilderContent("0123456789", builder);
+        builder.flexibleAppend("abcd");
+        expectBuilderContent("0123456789abcd", builder);
+        builder.flexibleAppend('e');
+        expectBuilderContent("0123456789abcde", builder);
+        builder.flexibleAppend("");
+        expectBuilderContent("0123456789abcde", builder);
+    }
+
+    {
+        StringBuilder builder;
+        builder.flexibleAppend(String("0123456789"), "abcd", 'e', "");
+        expectBuilderContent("0123456789abcde", builder);
+        builder.flexibleAppend(String("A"), "B", 'C', "");
+        expectBuilderContent("0123456789abcdeABC", builder);
+    }
+}
+
 TEST(StringBuilderTest, ToString)
 {
     StringBuilder builder;