Summary: | Simplify and clean SpaceSplitString | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Benjamin Poulain <benjamin> | ||||||||
Component: | New Bugs | Assignee: | Benjamin Poulain <benjamin> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, darin, esprehn+autocc, kangil.han, rniwa, sam | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Benjamin Poulain
2013-08-27 17:11:00 PDT
Created attachment 209824 [details]
Patch
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? 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. Created attachment 209842 [details]
Patch
Created attachment 209843 [details]
Patch
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. Committed r154780: <http://trac.webkit.org/changeset/154780> |