Attachment 373727 Details for Bug 198997 – Patch

[patch] Patch

bug-198997-20190709095623.patch (text/plain), 13.33 KB, created by Sam Weinig on 2019-07-09 09:56:24 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Sam Weinig

Created: 2019-07-09 09:56:24 PDT

Size: 13.33 KB

patch

obsolete

>Index: Source/WTF/ChangeLog
>===================================================================
>--- Source/WTF/ChangeLog	(revision 247258)
>+++ Source/WTF/ChangeLog	(working copy)
>@@ -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 NOBODY (OOPS!).
>+
>+        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-08  Chris Dumez  <cdumez@apple.com>
> 
>         Use WeakHashSet for WebUserContentControllerProxy::m_processes
>Index: Source/WTF/wtf/posix/FileSystemPOSIX.cpp
>===================================================================
>--- Source/WTF/wtf/posix/FileSystemPOSIX.cpp	(revision 247258)
>+++ Source/WTF/wtf/posix/FileSystemPOSIX.cpp	(working copy)
>@@ -301,10 +301,8 @@ String pathByAppendingComponents(StringV
> {
>     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: Source/WTF/wtf/text/StringBuilder.cpp
>===================================================================
>--- Source/WTF/wtf/text/StringBuilder.cpp	(revision 247258)
>+++ Source/WTF/wtf/text/StringBuilder.cpp	(working copy)
>@@ -231,21 +231,29 @@ void StringBuilder::reserveCapacity(unsi
>     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)
>+ALWAYS_INLINE CharType* 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<CharType>(requiredLength);
>+}
>+
>+template <typename CharType>
>+ALWAYS_INLINE CharType* 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>());
>@@ -258,7 +266,17 @@ ALWAYS_INLINE CharType* StringBuilder::a
>     return appendUninitializedSlow<CharType>(requiredLength.unsafeGet());
> }
> 
>-// Make 'length' additional capacity be available in m_buffer, update m_string & m_length,
>+UChar* StringBuilder::appendUninitializedWithoutOverflowCheckForUChar(CheckedInt32 requiredLength)
>+{
>+    return appendUninitializedWithoutOverflowCheck<UChar>(requiredLength);
>+}
>+
>+LChar* StringBuilder::appendUninitializedWithoutOverflowCheckForLChar(CheckedInt32 requiredLength)
>+{
>+    return appendUninitializedWithoutOverflowCheck<LChar>(requiredLength);
>+}
>+
>+// 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)
>Index: Source/WTF/wtf/text/StringBuilder.h
>===================================================================
>--- Source/WTF/wtf/text/StringBuilder.h	(revision 247258)
>+++ Source/WTF/wtf/text/StringBuilder.h	(working copy)
>@@ -231,6 +231,9 @@ public:
>     WTF_EXPORT_PRIVATE void appendFixedWidthNumber(float, unsigned decimalPlaces);
>     WTF_EXPORT_PRIVATE void appendFixedWidthNumber(double, unsigned decimalPlaces);
> 
>+    template<typename... StringTypes>
>+    void flexibleAppend(StringTypes ...strings);
>+
>     String toString()
>     {
>         if (!m_string.isNull()) {
>@@ -350,16 +353,21 @@ 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 additionalLength);
>+    template <typename CharType> ALWAYS_INLINE CharType* appendUninitializedWithoutOverflowCheck(CheckedInt32 requiredLength);
>+    template <typename CharType> CharType* appendUninitializedSlow(unsigned requiredLength);
>+    
>+    UChar* appendUninitializedWithoutOverflowCheckForUChar(CheckedInt32 requiredLength);
>+    LChar* appendUninitializedWithoutOverflowCheckForLChar(CheckedInt32 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();
>+    ALWAYS_INLINE CharType* getBufferCharacters();
>     WTF_EXPORT_PRIVATE void reifyString() const;
> 
>+    template<typename... StringTypeAdapters>
>+    void flexibleAppendFromAdapters(StringTypeAdapters ...adapters);
>+
>     mutable String m_string;
>     RefPtr<StringImpl> m_buffer;
>     union {
>@@ -388,6 +396,38 @@ ALWAYS_INLINE UChar* StringBuilder::getB
>     return m_bufferCharacters16;
> }
> 
>+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 CharType>
> bool equal(const StringBuilder& s, const CharType* buffer, unsigned length)
> {
>Index: Source/WTF/wtf/text/StringConcatenate.h
>===================================================================
>--- Source/WTF/wtf/text/StringConcatenate.h	(revision 247258)
>+++ Source/WTF/wtf/text/StringConcatenate.h	(working copy)
>@@ -248,16 +248,16 @@ inline bool are8Bit(Adapter adapter, Ada
> }
> 
> 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(StringT
>         if (!resultImpl)
>             return String();
> 
>-        makeStringAccumulator(buffer, adapter, adapters...);
>+        stringTypeAdapterAccumulator(buffer, adapter, adapters...);
> 
>         return resultImpl;
>     }
>@@ -286,7 +286,7 @@ String tryMakeStringFromAdapters(StringT
>     if (!resultImpl)
>         return String();
> 
>-    makeStringAccumulator(buffer, adapter, adapters...);
>+    stringTypeAdapterAccumulator(buffer, adapter, adapters...);
> 
>     return resultImpl;
> }
>Index: Tools/ChangeLog
>===================================================================
>--- Tools/ChangeLog	(revision 247258)
>+++ Tools/ChangeLog	(working copy)
>@@ -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 NOBODY (OOPS!).
>+
>+        * 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-08  Aakash Jain  <aakash_jain@apple.com>
> 
>         [ews-build] Do not run unix commands for windows in PrintConfiguration
>Index: Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp
>===================================================================
>--- Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp	(revision 247258)
>+++ Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp	(working copy)
>@@ -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;

Actions: View | Formatted Diff | Diff

Attachments on bug 198997: 372457 | 373727 | 373774 | 373782 | 374303 | 374315 | 374321