[webkit-reviews] review denied: [Bug 171586] NeverDestroyed<String>(ASCIILiteral(...)) is not thread safe. : [Attachment 308879] rebased to ToT

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 171586: NeverDestroyed<String>(ASCIILiteral(...)) is not thread safe.
https://bugs.webkit.org/show_bug.cgi?id=171586

Attachment 308879: rebased to ToT

https://bugs.webkit.org/attachment.cgi?id=308879&action=review




--- Comment #6 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 308879
  --> https://bugs.webkit.org/attachment.cgi?id=308879
rebased to ToT

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

r- because bug

> Source/JavaScriptCore/ChangeLog:16
> +	   deref is down in a thread-safe way, the NeverDestroyed<String> may
get destroyed

*done

>>> Source/WTF/ChangeLog:23
>>> +		 NeverDestroyed<String> myString(ASCIILiteral("myString"));
>> 
>> Why are there different ascii literal strings? Is this to allow non-atomic
increment of literal strings in the dom?
> 
> ASCIILiteral() is introduced to distinguish `const char*` from literal, it
can represent some good features of literals.
> For example, while `const char*` may be non-null-terminated, we can say that
ASCIILiteral() is always null-terminated.
> 
> I think the current StaticASCIILiteral has a bug since makeStaticStringImpl
does not work well. I suggest using StaticStringImpl directly in
NeverDestroyed<String> places.
> Like, `static StringImpl::StaticStringImpl myString("myString")`.

I agree.

>> Source/WTF/wtf/text/StringImpl.h:1164
>> +}
> 
> I think it's not correct. It returns the same StaticStringImpl& for
`makeStaticStringImpl("Hello")` and `makeStaticStringImpl("World")`
(strlen("Hello") == strlen("World")), right?

Agreed.