Unreviewed, rolling out r219510.

https://bugs.webkit.org/show_bug.cgi?id=174525

Need to revert length() == 0 check for null string (Requested
by yusukesuzuki on #webkit).

Reverted changeset:

"[WTF] Newly added AtomicStringImpl should use BufferInternal
static string if StringImpl is static"
https://bugs.webkit.org/show_bug.cgi?id=174501
http://trac.webkit.org/changeset/219510

Canonical link: https://commits.webkit.org/191343@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@219521 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WTF/ChangeLog

@@ -1,3 +1,18 @@
+2017-07-14  Commit Queue  <commit-queue@webkit.org>
+
+        Unreviewed, rolling out r219510.
+        https://bugs.webkit.org/show_bug.cgi?id=174525
+
+        Need to revert length() == 0 check for null string (Requested
+        by yusukesuzuki on #webkit).
+
+        Reverted changeset:
+
+        "[WTF] Newly added AtomicStringImpl should use BufferInternal
+        static string if StringImpl is static"
+        https://bugs.webkit.org/show_bug.cgi?id=174501
+        http://trac.webkit.org/changeset/219510
+
 2017-07-14  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [WTF] Newly added AtomicStringImpl should use BufferInternal static string if StringImpl is static

Source/WTF/wtf/text/AtomicStringImpl.cpp

@@ -95,70 +95,55 @@ static inline Ref<AtomicStringImpl> addToStringTable(const T& value)
 }
 
 struct CStringTranslator {
-    static unsigned hash(const LChar* characters)
+    static unsigned hash(const LChar* c)
     {
-        return StringHasher::computeHashAndMaskTop8Bits(characters);
+        return StringHasher::computeHashAndMaskTop8Bits(c);
     }
 
-    static inline bool equal(StringImpl* str, const LChar* characters)
+    static inline bool equal(StringImpl* r, const LChar* s)
     {
-        return WTF::equal(str, characters);
+        return WTF::equal(r, s);
     }
 
-    static void translate(StringImpl*& location, const LChar* const& characters, unsigned hash)
+    static void translate(StringImpl*& location, const LChar* const& c, unsigned hash)
     {
-        location = &StringImpl::create(characters).leakRef();
+        location = &StringImpl::create(c).leakRef();
         location->setHash(hash);
         location->setIsAtomic(true);
     }
 };
 
-RefPtr<AtomicStringImpl> AtomicStringImpl::add(const LChar* characters)
+RefPtr<AtomicStringImpl> AtomicStringImpl::add(const LChar* c)
 {
-    if (!characters)
+    if (!c)
         return nullptr;
-    if (!*characters)
+    if (!*c)
         return static_cast<AtomicStringImpl*>(StringImpl::empty());
 
-    return addToStringTable<const LChar*, CStringTranslator>(characters);
+    return addToStringTable<const LChar*, CStringTranslator>(c);
 }
 
 template<typename CharacterType>
 struct HashTranslatorCharBuffer {
-    const CharacterType* characters;
+    const CharacterType* s;
     unsigned length;
-    unsigned hash;
-
-    HashTranslatorCharBuffer(const CharacterType* characters, unsigned length)
-        : characters(characters)
-        , length(length)
-        , hash(StringHasher::computeHashAndMaskTop8Bits(characters, length))
-    {
-    }
-
-    HashTranslatorCharBuffer(const CharacterType* characters, unsigned length, unsigned hash)
-        : characters(characters)
-        , length(length)
-        , hash(hash)
-    {
-    }
 };
 
-using UCharBuffer = HashTranslatorCharBuffer<UChar>;
+typedef HashTranslatorCharBuffer<UChar> UCharBuffer;
 struct UCharBufferTranslator {
     static unsigned hash(const UCharBuffer& buf)
     {
-        return buf.hash;
+        return StringHasher::computeHashAndMaskTop8Bits(buf.s, buf.length);
     }
 
     static bool equal(StringImpl* const& str, const UCharBuffer& buf)
     {
-        return WTF::equal(str, buf.characters, buf.length);
+        return WTF::equal(str, buf.s, buf.length);
     }
 
     static void translate(StringImpl*& location, const UCharBuffer& buf, unsigned hash)
     {
-        location = &StringImpl::create8BitIfPossible(buf.characters, buf.length).leakRef();
+        location = &StringImpl::create8BitIfPossible(buf.s, buf.length).leakRef();
         location->setHash(hash);
         location->setIsAtomic(true);
     }
@@ -232,31 +217,31 @@ struct HashAndUTF8CharactersTranslator {
     }
 };
 
-RefPtr<AtomicStringImpl> AtomicStringImpl::add(const UChar* characters, unsigned length)
+RefPtr<AtomicStringImpl> AtomicStringImpl::add(const UChar* s, unsigned length)
 {
-    if (!characters)
+    if (!s)
         return nullptr;
 
     if (!length)
         return static_cast<AtomicStringImpl*>(StringImpl::empty());
 
-    UCharBuffer buffer { characters, length };
+    UCharBuffer buffer = { s, length };
     return addToStringTable<UCharBuffer, UCharBufferTranslator>(buffer);
 }
 
-RefPtr<AtomicStringImpl> AtomicStringImpl::add(const UChar* characters)
+RefPtr<AtomicStringImpl> AtomicStringImpl::add(const UChar* s)
 {
-    if (!characters)
+    if (!s)
         return nullptr;
 
     unsigned length = 0;
-    while (characters[length] != UChar(0))
+    while (s[length] != UChar(0))
         ++length;
 
     if (!length)
         return static_cast<AtomicStringImpl*>(StringImpl::empty());
 
-    UCharBuffer buffer { characters, length };
+    UCharBuffer buffer = { s, length };
     return addToStringTable<UCharBuffer, UCharBufferTranslator>(buffer);
 }
 
@@ -320,56 +305,55 @@ RefPtr<AtomicStringImpl> AtomicStringImpl::add(StringImpl* baseString, unsigned
     return addToStringTable<SubstringLocation, SubstringTranslator16>(buffer);
 }
 
-using LCharBuffer = HashTranslatorCharBuffer<LChar>;
+typedef HashTranslatorCharBuffer<LChar> LCharBuffer;
 struct LCharBufferTranslator {
     static unsigned hash(const LCharBuffer& buf)
     {
-        return buf.hash;
+        return StringHasher::computeHashAndMaskTop8Bits(buf.s, buf.length);
     }
 
     static bool equal(StringImpl* const& str, const LCharBuffer& buf)
     {
-        return WTF::equal(str, buf.characters, buf.length);
+        return WTF::equal(str, buf.s, buf.length);
     }
 
     static void translate(StringImpl*& location, const LCharBuffer& buf, unsigned hash)
     {
-        location = &StringImpl::create(buf.characters, buf.length).leakRef();
+        location = &StringImpl::create(buf.s, buf.length).leakRef();
         location->setHash(hash);
         location->setIsAtomic(true);
     }
 };
 
-template<typename CharType>
-struct BufferFromStaticDataTranslator {
-    using Buffer = HashTranslatorCharBuffer<CharType>;
-    static unsigned hash(const Buffer& buf)
+typedef HashTranslatorCharBuffer<char> CharBuffer;
+struct CharBufferFromLiteralDataTranslator {
+    static unsigned hash(const CharBuffer& buf)
     {
-        return buf.hash;
+        return StringHasher::computeHashAndMaskTop8Bits(reinterpret_cast<const LChar*>(buf.s), buf.length);
     }
 
-    static bool equal(StringImpl* const& str, const Buffer& buf)
+    static bool equal(StringImpl* const& str, const CharBuffer& buf)
     {
-        return WTF::equal(str, buf.characters, buf.length);
+        return WTF::equal(str, buf.s, buf.length);
     }
 
-    static void translate(StringImpl*& location, const Buffer& buf, unsigned hash)
+    static void translate(StringImpl*& location, const CharBuffer& buf, unsigned hash)
     {
-        location = &StringImpl::createWithoutCopying(buf.characters, buf.length).leakRef();
+        location = &StringImpl::createFromLiteral(buf.s, buf.length).leakRef();
         location->setHash(hash);
         location->setIsAtomic(true);
     }
 };
 
-RefPtr<AtomicStringImpl> AtomicStringImpl::add(const LChar* characters, unsigned length)
+RefPtr<AtomicStringImpl> AtomicStringImpl::add(const LChar* s, unsigned length)
 {
-    if (!characters)
+    if (!s)
         return nullptr;
 
     if (!length)
         return static_cast<AtomicStringImpl*>(StringImpl::empty());
 
-    LCharBuffer buffer { characters, length };
+    LCharBuffer buffer = { s, length };
     return addToStringTable<LCharBuffer, LCharBufferTranslator>(buffer);
 }
 
@@ -378,55 +362,34 @@ Ref<AtomicStringImpl> AtomicStringImpl::addLiteral(const char* characters, unsig
     ASSERT(characters);
     ASSERT(length);
 
-    LCharBuffer buffer { reinterpret_cast<const LChar*>(characters), length };
-    return addToStringTable<LCharBuffer, BufferFromStaticDataTranslator<LChar>>(buffer);
+    CharBuffer buffer = { characters, length };
+    return addToStringTable<CharBuffer, CharBufferFromLiteralDataTranslator>(buffer);
 }
 
-static Ref<AtomicStringImpl> addSymbol(AtomicStringTableLocker& locker, StringTableImpl& atomicStringTable, StringImpl& base)
+static inline Ref<AtomicStringImpl> addSubstring(AtomicStringTableLocker& locker, StringTableImpl& atomicStringTable, StringImpl& base)
 {
     ASSERT(base.length());
-    ASSERT(base.isSymbol());
+    ASSERT(base.isSymbol() || base.isStatic());
 
     SubstringLocation buffer = { &base, 0, base.length() };
     if (base.is8Bit())
         return addToStringTable<SubstringLocation, SubstringTranslator8>(locker, atomicStringTable, buffer);
     return addToStringTable<SubstringLocation, SubstringTranslator16>(locker, atomicStringTable, buffer);
 }
 
-static inline Ref<AtomicStringImpl> addSymbol(StringImpl& base)
+static inline Ref<AtomicStringImpl> addSubstring(StringImpl& base)
 {
     AtomicStringTableLocker locker;
-    return addSymbol(locker, stringTable(), base);
-}
-
-static Ref<AtomicStringImpl> addStatic(AtomicStringTableLocker& locker, StringTableImpl& atomicStringTable, StringImpl& base)
-{
-    ASSERT(base.length());
-    ASSERT(base.isStatic());
-
-    if (base.is8Bit()) {
-        LCharBuffer buffer { base.characters8(), base.length(), base.hash() };
-        return addToStringTable<LCharBuffer, BufferFromStaticDataTranslator<LChar>>(locker, atomicStringTable, buffer);
-    }
-    UCharBuffer buffer { base.characters16(), base.length(), base.hash() };
-    return addToStringTable<UCharBuffer, BufferFromStaticDataTranslator<UChar>>(locker, atomicStringTable, buffer);
-}
-
-static inline Ref<AtomicStringImpl> addStatic(StringImpl& base)
-{
-    AtomicStringTableLocker locker;
-    return addStatic(locker, stringTable(), base);
+    return addSubstring(locker, stringTable(), base);
 }
 
 Ref<AtomicStringImpl> AtomicStringImpl::addSlowCase(StringImpl& string)
 {
-    ASSERT_WITH_MESSAGE(string.length(), "Empty string should be already handled.");
-
-    if (string.isStatic())
-        return addStatic(string);
+    if (!string.length())
+        return *static_cast<AtomicStringImpl*>(StringImpl::empty());
 
-    if (string.isSymbol())
-        return addSymbol(string);
+    if (string.isSymbol() || string.isStatic())
+        return addSubstring(string);
 
     ASSERT_WITH_MESSAGE(!string.isAtomic(), "AtomicStringImpl should not hit the slow case if the string is already atomic.");
 
@@ -443,16 +406,12 @@ Ref<AtomicStringImpl> AtomicStringImpl::addSlowCase(StringImpl& string)
 
 Ref<AtomicStringImpl> AtomicStringImpl::addSlowCase(AtomicStringTable& stringTable, StringImpl& string)
 {
-    ASSERT_WITH_MESSAGE(string.length(), "Empty string should be already handled.");
-
-    if (string.isStatic()) {
-        AtomicStringTableLocker locker;
-        return addStatic(locker, stringTable.table(), string);
-    }
+    if (!string.length())
+        return *static_cast<AtomicStringImpl*>(StringImpl::empty());
 
-    if (string.isSymbol()) {
+    if (string.isSymbol() || string.isStatic()) {
         AtomicStringTableLocker locker;
-        return addSymbol(locker, stringTable.table(), string);
+        return addSubstring(locker, stringTable.table(), string);
     }
 
     ASSERT_WITH_MESSAGE(!string.isAtomic(), "AtomicStringImpl should not hit the slow case if the string is already atomic.");
@@ -523,7 +482,7 @@ RefPtr<AtomicStringImpl> AtomicStringImpl::lookUp(const UChar* characters, unsig
     AtomicStringTableLocker locker;
     auto& table = stringTable();
 
-    UCharBuffer buffer { characters, length };
+    UCharBuffer buffer = { characters, length };
     auto iterator = table.find<UCharBufferTranslator>(buffer);
     if (iterator != table.end())
         return static_cast<AtomicStringImpl*>(*iterator);

Source/WTF/wtf/text/StringImpl.h

@@ -55,7 +55,7 @@ class SymbolImpl;
 class SymbolRegistry;
 
 struct CStringTranslator;
-template<typename> struct BufferFromStaticDataTranslator;
+struct CharBufferFromLiteralDataTranslator;
 struct HashAndUTF8CharactersTranslator;
 struct LCharBufferTranslator;
 struct StringHash;
@@ -182,13 +182,12 @@ class StringImpl : private StringImplShape {
     friend struct WTF::CStringTranslator;
     template<typename CharacterType> friend struct WTF::HashAndCharactersTranslator;
     friend struct WTF::HashAndUTF8CharactersTranslator;
-    template<typename CharacterType> friend struct WTF::BufferFromStaticDataTranslator;
+    friend struct WTF::CharBufferFromLiteralDataTranslator;
     friend struct WTF::LCharBufferTranslator;
     friend struct WTF::SubstringTranslator;
     friend struct WTF::UCharBufferTranslator;
     friend class JSC::LLInt::Data;
     friend class JSC::LLIntOffsetsExtractor;
-    friend class AtomicStringImpl;
     friend class SymbolImpl;
     friend class RegisteredSymbolImpl;
 

Tools/ChangeLog

@@ -1,3 +1,18 @@
+2017-07-14  Commit Queue  <commit-queue@webkit.org>
+
+        Unreviewed, rolling out r219510.
+        https://bugs.webkit.org/show_bug.cgi?id=174525
+
+        Need to revert length() == 0 check for null string (Requested
+        by yusukesuzuki on #webkit).
+
+        Reverted changeset:
+
+        "[WTF] Newly added AtomicStringImpl should use BufferInternal
+        static string if StringImpl is static"
+        https://bugs.webkit.org/show_bug.cgi?id=174501
+        http://trac.webkit.org/changeset/219510
+
 2017-07-14  Jer Noble  <jer.noble@apple.com>
 
         [MSE] Removing samples when presentation order does not match decode order can cause bad behavior.

Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp

@@ -55,17 +55,6 @@ TEST(WTF, StringImplCreationFromLiteral)
     ASSERT_TRUE(equal(programmaticStringNoLength.get(), stringWithoutLengthLiteral));
     ASSERT_EQ(stringWithoutLengthLiteral, reinterpret_cast<const char*>(programmaticStringNoLength->characters8()));
     ASSERT_TRUE(programmaticStringNoLength->is8Bit());
-
-    // AtomicStringImpl from createFromLiteral should use the same underlying string.
-    auto atomicStringWithTemplate = AtomicStringImpl::add(stringWithTemplate.ptr());
-    ASSERT_TRUE(atomicStringWithTemplate->is8Bit());
-    ASSERT_EQ(atomicStringWithTemplate->characters8(), stringWithTemplate->characters8());
-    auto atomicProgrammaticString = AtomicStringImpl::add(programmaticString.ptr());
-    ASSERT_TRUE(atomicProgrammaticString->is8Bit());
-    ASSERT_EQ(atomicProgrammaticString->characters8(), programmaticString->characters8());
-    auto atomicProgrammaticStringNoLength = AtomicStringImpl::add(programmaticStringNoLength.ptr());
-    ASSERT_TRUE(atomicProgrammaticStringNoLength->is8Bit());
-    ASSERT_EQ(atomicProgrammaticStringNoLength->characters8(), programmaticStringNoLength->characters8());
 }
 
 TEST(WTF, StringImplReplaceWithLiteral)
@@ -620,12 +609,8 @@ TEST(WTF, StringImplStaticToAtomicString)
     ASSERT_FALSE(original.isAtomic());
     ASSERT_TRUE(original.isStatic());
 
-    ASSERT_TRUE(atomic->is8Bit());
-    ASSERT_EQ(atomic->characters8(), original.characters8());
-
     auto result2 = AtomicStringImpl::lookUp(&original);
     ASSERT_TRUE(result2);
-    ASSERT_EQ(atomic, result2);
 }
 
 TEST(WTF, StringImplConstexprHasher)