Bug 166586 – Use StaticStringImpl instead of StaticASCIILiteral

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

[Yusuke Suzuki]

Use StaticStringImpl instead of StaticASCIILiteral

[Yusuke Suzuki]

Created attachment 297836 [details]
Patch

[Build Bot]

Comment on attachment 297836 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 297837 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

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

[Darin Adler]

Comment on attachment 297836 [details]
Patch

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

Any theory on the mac-debug test failures? I am slightly concerned.

> Source/WebCore/bindings/scripts/StaticString.pm:40
>          push(@result, <<END);
> -static StringImpl::StaticASCIILiteral ${name}Data = {
> -    StringImpl::StaticASCIILiteral::s_initialRefCount,
> -    $length,
> -    ${name}String8,
> -    StringImpl::StaticASCIILiteral::s_initialFlags | (${hash} << StringImpl::StaticASCIILiteral::s_hashShift)
> -};
> +static StringImpl::StaticStringImpl ${name}Data(\"${value}\");
>  END

Now that this is generating only a single line, I don’t think we need to use the "<<END" syntax any more. Just this will do:

    push(@result, "static StringImpl::StaticStringImpl ${name}Data(\"${value}\");\n");

[Yusuke Suzuki]

Comment on attachment 297836 [details]
Patch

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

Previous StaticASCIILiteral has severe problem: It does not use s_refCountFlagIsStaticString.
It means that these names should not be touched from the multiple threads. I think this policy is not broken unintentionally. But it is dangerous.

So, we should handle isStatic strings correctly in the StringImpl systems. Previously, if we attept to make static string impl atomic, it causes CRASH! This is the reason why the mac-debug crashses. We should fix it by handling it like SymbolImpl's isSymbol() case.

>> Source/WebCore/bindings/scripts/StaticString.pm:40
>>  END
> 
> Now that this is generating only a single line, I don’t think we need to use the "<<END" syntax any more. Just this will do:
> 
>     push(@result, "static StringImpl::StaticStringImpl ${name}Data(\"${value}\");\n");

Sounds nice. Fixed.

[Yusuke Suzuki]

Created attachment 297883 [details]
Patch

Patch for landing

[Yusuke Suzuki]

Created attachment 297889 [details]
Patch

Patch for landing

[Yusuke Suzuki]

Created attachment 297905 [details]
Patch

Patch for landing

[Yusuke Suzuki]

Created attachment 297907 [details]
Patch

Patch for landing

[Yusuke Suzuki]

Committed r210227: <http://trac.webkit.org/changeset/210227>

[Yusuke Suzuki]

*** Bug 165134 has been marked as a duplicate of this bug. ***

[Darin Adler]

Comment on attachment 297907 [details]
Patch

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

> Source/WebCore/bindings/scripts/StaticString.pm:43
> +    push(@result, <<END);
> +#if COMPILER(MSVC)
> +#pragma warning(push)
> +#pragma warning(disable: 4307)
> +#endif
> +END
>  
>      push(@result, "\n");

I would have merged these two. Just another blank line before END.

> Source/WebCore/bindings/scripts/StaticString.pm:56
>      push(@result, "\n");
>  
> +    push(@result, <<END);
> +#if COMPILER(MSVC)
> +#pragma warning(pop)
> +#endif
> +END

I would have merged these two. Just a blank line before the #if line.

[Yusuke Suzuki]

Comment on attachment 297907 [details]
Patch

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

Thanks, I'll land the follow-up patch for that!

>> Source/WebCore/bindings/scripts/StaticString.pm:43
>>      push(@result, "\n");
> 
> I would have merged these two. Just another blank line before END.

Sounds nice. Fixed in the follow up patch.

>> Source/WebCore/bindings/scripts/StaticString.pm:56
>> +END
> 
> I would have merged these two. Just a blank line before the #if line.

Sounds nice. Fixed in the follow up patch.

[Yusuke Suzuki]

Comment on attachment 297907 [details]
Patch

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

>>> Source/WebCore/bindings/scripts/StaticString.pm:56
>>> +END
>> 
>> I would have merged these two. Just a blank line before the #if line.
> 
> Sounds nice. Fixed in the follow up patch.

And add one more blank line after this #endif to fix the output style.

[Yusuke Suzuki]

Committed r210231: <http://trac.webkit.org/changeset/210231>