Bug 206802 – Add some tests for dynamically allocated StaticStringImpls.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Also address some feedback from Darin.  See https://bugs.webkit.org/show_bug.cgi?id=206791#c12 and https://bugs.webkit.org/show_bug.cgi?id=206791#c13.

[Mark Lam]

Created attachment 388782 [details]
proposed patch.

[Darin Adler]

Comment on attachment 388782 [details]
proposed patch.

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

Looks good! Thanks for doing this.

> Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp:728
> +    ASSERT_NE(hello.hash(), 0u);

We could go further with this check, and check that the hash is the canonical hash of that string that we use across WebKit. That could be done one of three ways:

1) Comparing hello.hash() with the result of hello.impl()->concurrentHash().
2) Comparing hello.hash() with the result of a call to StringHasher::computeHashAndMaskTop8Bits on the same literal characters "hello", 5.
3) Hard-coding the expected hash value of "hello".

Any of those three would be slightly better than just checking for the specific mistake of 0. In the StringHasher tests we used approach (3).

[Mark Lam]

Thanks for the review.

(In reply to Darin Adler from comment #2)
> 1) Comparing hello.hash() with the result of hello.impl()->concurrentHash().
> 2) Comparing hello.hash() with the result of a call to
> StringHasher::computeHashAndMaskTop8Bits on the same literal characters
> "hello", 5.
> 3) Hard-coding the expected hash value of "hello".
> 
> Any of those three would be slightly better than just checking for the
> specific mistake of 0. In the StringHasher tests we used approach (3).

I've applied (3): compare against the expected constant hash value.

Landed in r255125: <http://trac.webkit.org/r255125>.

[Radar WebKit Bug Importer]

<rdar://problem/58896315>