Bug 206802 - Add some tests for dynamically allocated StaticStringImpls.
Summary: Add some tests for dynamically allocated StaticStringImpls.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-01-25 10:55 PST by Mark Lam
Modified: 2020-01-25 12:16 PST (History)
8 users (show)

See Also:


Attachments
proposed patch. (6.37 KB, patch)
2020-01-25 11:06 PST, Mark Lam
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2020-01-25 10:55:08 PST
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.
Comment 1 Mark Lam 2020-01-25 11:06:20 PST
Created attachment 388782 [details]
proposed patch.
Comment 2 Darin Adler 2020-01-25 11:52:35 PST
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).
Comment 3 Mark Lam 2020-01-25 12:15:06 PST
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>.
Comment 4 Radar WebKit Bug Importer 2020-01-25 12:16:25 PST
<rdar://problem/58896315>