Bug 120385 – Simplify and clean SpaceSplitString

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

Simplify and clean SpaceSplitString

[Benjamin Poulain]

Created attachment 209824 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 209824 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209824&action=review

> Source/WebCore/dom/SpaceSplitString.cpp:32
> +COMPILE_ASSERT(!(sizeof(SpaceSplitStringData) % sizeof(void*)), SpaceSplitStringDataTailIsAlignedToWordSize);

sizeof(uint_ptr)

> Source/WebCore/dom/SpaceSplitString.cpp:93
> +    unsigned otherSize = other.m_size;

Maybe we should assert that other.m_size is non-zero?

> Source/WebCore/dom/SpaceSplitString.cpp:97
> +        const AtomicString& name = other[i];
> +        if (!contains(name))

No need for the temporary.

> Source/WebCore/dom/SpaceSplitString.cpp:106
> +    static const unsigned typicalNumberOfSpaceSplitString = 100;

Maybe we should comment how you got this number.

> Source/WebCore/dom/SpaceSplitString.cpp:173
> +class TokenCounterProcessor {
> +public:
> +    TokenCounterProcessor() : m_tokenCount(0) { }

Maybe it's better to call this TokenCounter?  That way, counter will stand out.

> Source/WebCore/dom/SpaceSplitString.cpp:188
> +class TokenInitializerProcessor {

TokenAtomicStringInitializer?

> Source/WebCore/dom/SpaceSplitString.cpp:235
> +    new (NotNull, spaceSplitStringData) SpaceSplitStringData(keyString, tokenCount);
> +    AtomicString* tokenArrayStart = spaceSplitStringData->tokenArrayStart();
> +    TokenInitializerProcessor tokenInitializer(tokenArrayStart);
> +    tokenizeSpaceSplitString(tokenInitializer, keyString);
> +    ASSERT(tokenInitializer.nextMemoryBucket() - tokenArrayStart == tokenCount);
> +    ASSERT(reinterpret_cast<const char*>(tokenInitializer.nextMemoryBucket()) == (reinterpret_cast<const char*>(spaceSplitStringData) + sizeToAllocate));

Maybe we should extract this part as a function?

[Darin Adler]

Comment on attachment 209824 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209824&action=review

> Source/WebCore/dom/SpaceSplitString.cpp:-178
> -    if (inputString.isNull()) {
> -        clear();
> +    if (inputString.isNull())
>          return;
> -    }

Why the behavior change? Now passing in a null string results in no change!?

> Source/WebCore/dom/SpaceSplitString.cpp:125
> +        string = string.string().foldCase();

Do we really want to make an AtomicString out of the string in this case? What’s the benefit?

>> Source/WebCore/dom/SpaceSplitString.cpp:188
>> +class TokenInitializerProcessor {
> 
> TokenAtomicStringInitializer?

Should mark this non-copyable.

> Source/WebCore/dom/SpaceSplitString.cpp:225
> +    RELEASE_ASSERT(tokenCount < ((std::numeric_limits<unsigned>::max() - sizeof(SpaceSplitStringData)) / sizeof(AtomicString)));

I would have omitted the parentheses here.

>> Source/WebCore/dom/SpaceSplitString.cpp:235
>> +    ASSERT(reinterpret_cast<const char*>(tokenInitializer.nextMemoryBucket()) == (reinterpret_cast<const char*>(spaceSplitStringData) + sizeToAllocate));
> 
> Maybe we should extract this part as a function?

I would have omitted the parentheses here.

> Source/WebCore/dom/SpaceSplitString.h:-30
> -    class SpaceSplitStringData : public RefCounted<SpaceSplitStringData> {

Could you land the indenting change separately first? I can’t see the diff because the reinventing makes it look like every line changed.

[Benjamin Poulain]

Created attachment 209842 [details]
Patch

[Benjamin Poulain]

Created attachment 209843 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 209843 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209843&action=review

> Source/WebCore/dom/SpaceSplitString.cpp:240
> +    new (NotNull, spaceSplitStringData) SpaceSplitStringData(keyString, tokenCount);
> +    AtomicString* tokenArrayStart = spaceSplitStringData->tokenArrayStart();
> +    TokenAtomicStringInitializer tokenInitializer(tokenArrayStart);
> +    tokenizeSpaceSplitString(tokenInitializer, keyString);
> +    ASSERT(tokenInitializer.nextMemoryBucket() - tokenArrayStart == tokenCount);
> +    ASSERT(reinterpret_cast<const char*>(tokenInitializer.nextMemoryBucket()) == reinterpret_cast<const char*>(spaceSplitStringData) + sizeToAllocate);

I would really like to split this into a tiny inline function.

> Source/WebCore/dom/SpaceSplitString.h:67
> +        ASSERT(isMainThread());

We should also assert that m_refCount is non-zero here.

[Benjamin Poulain]

Committed r154780: <http://trac.webkit.org/changeset/154780>