Bug 165247 – Refactor SymbolImpl layout

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Yusuke Suzuki]

Let's put field in SymbolImpl. And add a flag for Null symbol (`Symbol()`).

[Yusuke Suzuki]

Created attachment 295830 [details]
Patch

[WebKit Commit Bot]

Attachment 295830 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 3 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 295831 [details]
Patch

[Yusuke Suzuki]

Created attachment 295832 [details]
Patch

[WebKit Commit Bot]

Attachment 295832 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 3 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 295836 [details]
Patch

[WebKit Commit Bot]

Attachment 295836 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 3 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 295837 [details]
Patch

[WebKit Commit Bot]

Attachment 295837 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 3 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 295838 [details]
Patch

[WebKit Commit Bot]

Attachment 295838 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 3 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 295839 [details]
Patch

[WebKit Commit Bot]

Attachment 295839 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 3 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 295840 [details]
Patch

[WebKit Commit Bot]

Attachment 295840 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 3 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 295840 [details]
Patch

Attachment 295840 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2601095

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 295848 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Yusuke Suzuki]

Created attachment 295927 [details]
Patch

[WebKit Commit Bot]

Attachment 295927 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:40:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/UniquedStringImpl.h:41:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 3 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

And this also paves the way for static SymbolImpl.

[Darin Adler]

Comment on attachment 295927 [details]
Patch

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

> Source/WTF/wtf/text/StringImpl.cpp:118
> +        SymbolImpl& symbol = static_cast<SymbolImpl&>(*this);

I would use auto& here.

> Source/WTF/wtf/text/StringImpl.cpp:120
> +        if (symbol.symbolRegistry())
> +            symbol.symbolRegistry()->remove(symbol);

I typically would put this into a scoped local variable, but not everyone agrees and I think it’s likely that the generated code will be identical.

> Source/WTF/wtf/text/StringImpl.h:732
> +    enum CreateSymbolTag { CreateSymbol };
> +    // Used to create new symbol strings that holds existing 8-bit [[Description]] string as a substring buffer (BufferSubstring).

I suggest a blank line here after the enum.

> Source/WTF/wtf/text/SymbolImpl.cpp:56
> +    return adoptRef(*new SymbolImpl());

No need for () here after SymbolImpl.

> Source/WTF/wtf/text/SymbolImpl.h:109
> +}
> +
> +
> +
>  #if !ASSERT_DISABLED

Should just have one blank line, not three.

[Yusuke Suzuki]

Comment on attachment 295927 [details]
Patch

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

>> Source/WTF/wtf/text/StringImpl.cpp:118
>> +        SymbolImpl& symbol = static_cast<SymbolImpl&>(*this);
> 
> I would use auto& here.

Fixed.

>> Source/WTF/wtf/text/StringImpl.cpp:120
>> +            symbol.symbolRegistry()->remove(symbol);
> 
> I typically would put this into a scoped local variable, but not everyone agrees and I think it’s likely that the generated code will be identical.

OK, i'll change this to `auto* symbolRegistry = symbol.symbolRegistry()`.

>> Source/WTF/wtf/text/StringImpl.h:732
>> +    // Used to create new symbol strings that holds existing 8-bit [[Description]] string as a substring buffer (BufferSubstring).
> 
> I suggest a blank line here after the enum.

Inserted.

>> Source/WTF/wtf/text/SymbolImpl.cpp:56
>> +    return adoptRef(*new SymbolImpl());
> 
> No need for () here after SymbolImpl.

Dropped.

>> Source/WTF/wtf/text/SymbolImpl.h:109
>>  #if !ASSERT_DISABLED
> 
> Should just have one blank line, not three.

Ooops. Fixed.

[Yusuke Suzuki]

Committed r209309: <http://trac.webkit.org/changeset/209309>