| Summary: | Add some tests for dynamically allocated StaticStringImpls. | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||
| Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, darin, dbates, ews-watchlist, webkit-bug-importer, ysuzuki | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Mark Lam
2020-01-25 10:55:08 PST
Created attachment 388782 [details]
proposed patch.
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). 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>. |