206802 2020-01-25 10:55:08 -0800 Add some tests for dynamically allocated StaticStringImpls. 2020-01-25 12:16:25 -0800 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 mark.lam mark.lam benjamin cdumez cmarcelo darin dbates ews-watchlist webkit-bug-importer ysuzuki oldest_to_newest 1611073 0 mark.lam 2020-01-25 10:55:08 -0800 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. 1611080 1 388782 mark.lam 2020-01-25 11:06:20 -0800 Created attachment 388782 proposed patch. 1611087 2 388782 darin 2020-01-25 11:52:35 -0800 Comment on attachment 388782 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). 1611095 3 mark.lam 2020-01-25 12:15:06 -0800 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>. 1611096 4 webkit-bug-importer 2020-01-25 12:16:25 -0800 <rdar://problem/58896315> 388782 2020-01-25 11:06:20 -0800 2020-01-25 11:52:47 -0800 proposed patch. bug-206802.patch text/plain 6524 mark.lam SW5kZXg6IFNvdXJjZS9XVEYvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvQ2hh bmdlTG9nCShyZXZpc2lvbiAyNTUxMjApCisrKyBTb3VyY2UvV1RGL0NoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBACisyMDIwLTAxLTI1ICBNYXJrIExhbSAgPG1hcmsubGFt QGFwcGxlLmNvbT4KKworICAgICAgICBBZGQgc29tZSB0ZXN0cyBmb3IgZHluYW1pY2FsbHkgYWxs b2NhdGVkIFN0YXRpY1N0cmluZ0ltcGxzLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9MjA2ODAyCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgUmVtb3ZlZCBzb21lIHVubmVjZXNzYXJ5IGV4cGxpY2l0IHNwZWNp YWxpemF0aW9uIG9mIHRoZSBjaGFyYWN0ZXJzQXJlQWxsQVNDSUkoKQorICAgICAgICB0ZW1wbGF0 ZSBmdW5jdGlvbi4KKworICAgICAgICAqIHd0Zi90ZXh0L1N0cmluZ0ltcGwuY3BwOgorICAgICAg ICAoV1RGOjpTdHJpbmdJbXBsOjpjcmVhdGVGcm9tTGl0ZXJhbCk6CisgICAgICAgIChXVEY6OlN0 cmluZ0ltcGw6OmNyZWF0ZVN0YXRpY1N0cmluZ0ltcGwpOgorCiAyMDIwLTAxLTI0ICBNYXJrIExh bSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KIAogICAgICAgICBNb3ZlIHNpbmdsZXRvbiBJbnRsIHN0 cmluZyBsb2NhbGVzIG91dCBvZiBKU0dsb2JhbE9iamVjdC4KSW5kZXg6IFNvdXJjZS9XVEYvd3Rm L3RleHQvU3RyaW5nSW1wbC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dURi93dGYvdGV4dC9T dHJpbmdJbXBsLmNwcAkocmV2aXNpb24gMjU1MTIwKQorKysgU291cmNlL1dURi93dGYvdGV4dC9T dHJpbmdJbXBsLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMTU2LDcgKzE1Niw3IEBAIHZvaWQgU3Ry aW5nSW1wbDo6ZGVzdHJveShTdHJpbmdJbXBsKiBzdHIKIFJlZjxTdHJpbmdJbXBsPiBTdHJpbmdJ bXBsOjpjcmVhdGVGcm9tTGl0ZXJhbChjb25zdCBjaGFyKiBjaGFyYWN0ZXJzLCB1bnNpZ25lZCBs ZW5ndGgpCiB7CiAgICAgQVNTRVJUX1dJVEhfTUVTU0FHRShsZW5ndGgsICJVc2UgU3RyaW5nSW1w bDo6ZW1wdHkoKSB0byBjcmVhdGUgYW4gZW1wdHkgc3RyaW5nIik7Ci0gICAgQVNTRVJUKGNoYXJh Y3RlcnNBcmVBbGxBU0NJSTxMQ2hhcj4ocmVpbnRlcnByZXRfY2FzdDxjb25zdCBMQ2hhcio+KGNo YXJhY3RlcnMpLCBsZW5ndGgpKTsKKyAgICBBU1NFUlQoY2hhcmFjdGVyc0FyZUFsbEFTQ0lJKHJl aW50ZXJwcmV0X2Nhc3Q8Y29uc3QgTENoYXIqPihjaGFyYWN0ZXJzKSwgbGVuZ3RoKSk7CiAgICAg cmV0dXJuIGFkb3B0UmVmKCpuZXcgU3RyaW5nSW1wbChyZWludGVycHJldF9jYXN0PGNvbnN0IExD aGFyKj4oY2hhcmFjdGVycyksIGxlbmd0aCwgQ29uc3RydWN0V2l0aG91dENvcHlpbmcpKTsKIH0K IApAQCAtMjg0LDcgKzI4NCw3IEBAIFJlZjxTdHJpbmdJbXBsPiBTdHJpbmdJbXBsOjpjcmVhdGUo Y29uc3QKIFJlZjxTdHJpbmdJbXBsPiBTdHJpbmdJbXBsOjpjcmVhdGVTdGF0aWNTdHJpbmdJbXBs KGNvbnN0IGNoYXIqIGNoYXJhY3RlcnMsIHVuc2lnbmVkIGxlbmd0aCkKIHsKICAgICBjb25zdCBM Q2hhciogbGNoYXJDaGFyYWN0ZXJzID0gcmVpbnRlcnByZXRfY2FzdDxjb25zdCBMQ2hhcio+KGNo YXJhY3RlcnMpOwotICAgIEFTU0VSVChjaGFyYWN0ZXJzQXJlQWxsQVNDSUk8TENoYXI+KGxjaGFy Q2hhcmFjdGVycywgbGVuZ3RoKSk7CisgICAgQVNTRVJUKGNoYXJhY3RlcnNBcmVBbGxBU0NJSShs Y2hhckNoYXJhY3RlcnMsIGxlbmd0aCkpOwogICAgIFJlZjxTdHJpbmdJbXBsPiByZXN1bHQgPSBj cmVhdGVJbnRlcm5hbChsY2hhckNoYXJhY3RlcnMsIGxlbmd0aCk7CiAgICAgcmVzdWx0LT5zZXRI YXNoKFN0cmluZ0hhc2hlcjo6Y29tcHV0ZUhhc2hBbmRNYXNrVG9wOEJpdHMobGNoYXJDaGFyYWN0 ZXJzLCBsZW5ndGgpKTsKICAgICByZXN1bHQtPm1fcmVmQ291bnQgfD0gc19yZWZDb3VudEZsYWdJ c1N0YXRpY1N0cmluZzsKSW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29s cy9DaGFuZ2VMb2cJKHJldmlzaW9uIDI1NTEyMSkKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBACisyMDIwLTAxLTI1ICBNYXJrIExhbSAgPG1hcmsubGFt QGFwcGxlLmNvbT4KKworICAgICAgICBBZGQgc29tZSB0ZXN0cyBmb3IgZHluYW1pY2FsbHkgYWxs b2NhdGVkIFN0YXRpY1N0cmluZ0ltcGxzLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9MjA2ODAyCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgKiBUZXN0V2ViS2l0QVBJL1Rlc3RzL1dURi9TdHJpbmdJbXBsLmNw cDoKKyAgICAgICAgKFRlc3RXZWJLaXRBUEk6OmRvU3RhdGljU3RyaW5nSW1wbFRlc3RzKToKKyAg ICAgICAgKFRlc3RXZWJLaXRBUEk6OlRFU1QpOgorCiAyMDIwLTAxLTI1ICBQZXIgQXJuZSBWb2xs YW4gIDxwdm9sbGFuQGFwcGxlLmNvbT4KIAogICAgICAgICBbQ29jb2FdIE1lZGlhIG1pbWUgdHlw ZXMgbWFwIHNob3VsZCBiZSBjcmVhdGVkIGluIHRoZSBVSSBwcm9jZXNzCkluZGV4OiBUb29scy9U ZXN0V2ViS2l0QVBJL1Rlc3RzL1dURi9TdHJpbmdJbXBsLmNwcAo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29s cy9UZXN0V2ViS2l0QVBJL1Rlc3RzL1dURi9TdHJpbmdJbXBsLmNwcAkocmV2aXNpb24gMjU1MTE5 KQorKysgVG9vbHMvVGVzdFdlYktpdEFQSS9UZXN0cy9XVEYvU3RyaW5nSW1wbC5jcHAJKHdvcmtp bmcgY29weSkKQEAgLTY4NiwxNCArNjg2LDEzIEBAIHN0YXRpYyBjb25zdCBTdHJpbmcmIGdldE5l dmVyRGVzdHJveWVkU3QKICAgICByZXR1cm4gbmV2ZXJEZXN0cm95ZWRTdHJpbmcoKTsKIH07CiAK LVRFU1QoV1RGLCBTdGF0aWNTdHJpbmdJbXBsKQotewotICAgIC8vIENvbnN0cnVjdCB1c2luZyBN QUtFX1NUQVRJQ19TVFJJTkdfSU1QTC4KLSAgICBTdHJpbmcgaGVsbG8oTUFLRV9TVEFUSUNfU1RS SU5HX0lNUEwoImhlbGxvIikpOwotICAgIFN0cmluZyB3b3JsZChNQUtFX1NUQVRJQ19TVFJJTkdf SU1QTCgid29ybGQiKSk7Ci0gICAgU3RyaW5nIGxvbmdlcihNQUtFX1NUQVRJQ19TVFJJTkdfSU1Q TCgibG9uZ2VyIikpOwotICAgIFN0cmluZyBoZWxsbzIoTUFLRV9TVEFUSUNfU1RSSU5HX0lNUEwo ImhlbGxvIikpOworZW51bSBjbGFzcyBTdGF0aWNTdHJpbmdJbXBsVGVzdFNldCB7CisgICAgU3Rh dGljYWxseUFsbG9jYXRlZEltcGwsCisgICAgRHluYW1pY2FsbHlBbGxvY2F0ZWRJbXBsCit9Owog CitzdGF0aWMgdm9pZCBkb1N0YXRpY1N0cmluZ0ltcGxUZXN0cyhTdGF0aWNTdHJpbmdJbXBsVGVz dFNldCB0ZXN0U2V0LCBTdHJpbmcmIGhlbGxvLCBTdHJpbmcmIHdvcmxkLCBTdHJpbmcmIGxvbmdl ciwgU3RyaW5nJiBoZWxsbzIpCit7CiAgICAgQVNTRVJUX0VRKHN0cmxlbigiaGVsbG8iKSwgaGVs bG8ubGVuZ3RoKCkpOwogICAgIEFTU0VSVF9FUShzdHJsZW4oIndvcmxkIiksIHdvcmxkLmxlbmd0 aCgpKTsKICAgICBBU1NFUlRfRVEoc3RybGVuKCJsb25nZXIiKSwgbG9uZ2VyLmxlbmd0aCgpKTsK QEAgLTcwNywxOCArNzA2LDQ4IEBAIFRFU1QoV1RGLCBTdGF0aWNTdHJpbmdJbXBsKQogICAgIC8v IEVhY2ggU3RhdGljU3RyaW5nSW1wbCogcmV0dXJuZWQgYnkgTUFLRV9TVEFUSUNfU1RSSU5HX0lN UEwgc2hvdWxkIGJlIHVuaXF1ZS4KICAgICBBU1NFUlRfTkUoaGVsbG8uaW1wbCgpLCBoZWxsbzIu aW1wbCgpKTsKIAotICAgIC8vIFRlc3QgdGhhdCBNQUtFX1NUQVRJQ19TVFJJTkdfSU1QTCBpc24n dCBhbGxvY2F0aW5nIGEgU3RhdGljU3RyaW5nSW1wbCBvbiB0aGUgc3RhY2suCi0gICAgY29uc3Qg U3RyaW5nJiBzdHIxID0gZ2V0TmV2ZXJEZXN0cm95ZWRTdHJpbmdBdFN0YWNrRGVwdGgoMTApOwot ICAgIEFTU0VSVF9FUShzdHJsZW4oIk5ldmVyRGVzdHJveWVkU3RyaW5nIiksIHN0cjEubGVuZ3Ro KCkpOwotICAgIEFTU0VSVF9UUlVFKGVxdWFsKHN0cjEsICJOZXZlckRlc3Ryb3llZFN0cmluZyIp KTsKLQotICAgIGNvbnN0IFN0cmluZyYgc3RyMiA9IGdldE5ldmVyRGVzdHJveWVkU3RyaW5nQXRT dGFja0RlcHRoKDIwKTsKLSAgICBBU1NFUlRfRVEoc3RybGVuKCJOZXZlckRlc3Ryb3llZFN0cmlu ZyIpLCBzdHIyLmxlbmd0aCgpKTsKLSAgICBBU1NFUlRfVFJVRShlcXVhbChzdHIyLCAiTmV2ZXJE ZXN0cm95ZWRTdHJpbmciKSk7Ci0KLSAgICBBU1NFUlRfVFJVRShlcXVhbChzdHIxLCBzdHIyKSk7 Ci0gICAgQVNTRVJUX0VRKCZzdHIxLCAmc3RyMik7Ci0gICAgQVNTRVJUX0VRKHN0cjEuaW1wbCgp LCBzdHIyLmltcGwoKSk7CisgICAgaWYgKHRlc3RTZXQgPT0gU3RhdGljU3RyaW5nSW1wbFRlc3RT ZXQ6OlN0YXRpY2FsbHlBbGxvY2F0ZWRJbXBsKSB7CisgICAgICAgIC8vIFRlc3QgdGhhdCBNQUtF X1NUQVRJQ19TVFJJTkdfSU1QTCBpc24ndCBhbGxvY2F0aW5nIGEgU3RhdGljU3RyaW5nSW1wbCBv biB0aGUgc3RhY2suCisgICAgICAgIGNvbnN0IFN0cmluZyYgc3RyMSA9IGdldE5ldmVyRGVzdHJv eWVkU3RyaW5nQXRTdGFja0RlcHRoKDEwKTsKKyAgICAgICAgQVNTRVJUX0VRKHN0cmxlbigiTmV2 ZXJEZXN0cm95ZWRTdHJpbmciKSwgc3RyMS5sZW5ndGgoKSk7CisgICAgICAgIEFTU0VSVF9UUlVF KGVxdWFsKHN0cjEsICJOZXZlckRlc3Ryb3llZFN0cmluZyIpKTsKKworICAgICAgICBjb25zdCBT dHJpbmcmIHN0cjIgPSBnZXROZXZlckRlc3Ryb3llZFN0cmluZ0F0U3RhY2tEZXB0aCgyMCk7Cisg ICAgICAgIEFTU0VSVF9FUShzdHJsZW4oIk5ldmVyRGVzdHJveWVkU3RyaW5nIiksIHN0cjIubGVu Z3RoKCkpOworICAgICAgICBBU1NFUlRfVFJVRShlcXVhbChzdHIyLCAiTmV2ZXJEZXN0cm95ZWRT dHJpbmciKSk7CisKKyAgICAgICAgQVNTRVJUX1RSVUUoZXF1YWwoc3RyMSwgc3RyMikpOworICAg ICAgICBBU1NFUlRfRVEoJnN0cjEsICZzdHIyKTsKKyAgICAgICAgQVNTRVJUX0VRKHN0cjEuaW1w bCgpLCBzdHIyLmltcGwoKSk7CisgICAgfQorCisgICAgLy8gVGVzdCB0aGF0IHRoZSBTdGF0aWNT dHJpbmdJbXBsJ3MgaGFzaCBoYXMgYWxyZWFkeSBiZWVuIHNldC4KKyAgICAvLyBXZSdyZSByZWx5 aW5nIG9uIGFuIEFTU0VSVCBpbiBzZXRIYXNoKCkgdG8gZGV0ZWN0IHRoYXQgdGhlIGhhc2ggaGFz bid0CisgICAgLy8gYWxyZWFkeSBiZWVuIHNldC4gSWYgdGhlIGhhc2ggaGFzIGFscmVhZHkgYmVl biBzZXQsIHRoZSBoYXNoKCkgbWV0aG9kCisgICAgLy8gd2lsbCBub3QgY2FsbCBzZXRIYXNoKCku CisgICAgQVNTRVJUX05FKGhlbGxvLmhhc2goKSwgMHUpOworfQorCitURVNUKFdURiwgU3RhdGlj U3RyaW5nSW1wbCkKK3sKKyAgICAvLyBDb25zdHJ1Y3QgdXNpbmcgTUFLRV9TVEFUSUNfU1RSSU5H X0lNUEwuCisgICAgU3RyaW5nIGhlbGxvKE1BS0VfU1RBVElDX1NUUklOR19JTVBMKCJoZWxsbyIp KTsKKyAgICBTdHJpbmcgd29ybGQoTUFLRV9TVEFUSUNfU1RSSU5HX0lNUEwoIndvcmxkIikpOwor ICAgIFN0cmluZyBsb25nZXIoTUFLRV9TVEFUSUNfU1RSSU5HX0lNUEwoImxvbmdlciIpKTsKKyAg ICBTdHJpbmcgaGVsbG8yKE1BS0VfU1RBVElDX1NUUklOR19JTVBMKCJoZWxsbyIpKTsKKworICAg IGRvU3RhdGljU3RyaW5nSW1wbFRlc3RzKFN0YXRpY1N0cmluZ0ltcGxUZXN0U2V0OjpTdGF0aWNh bGx5QWxsb2NhdGVkSW1wbCwgaGVsbG8sIHdvcmxkLCBsb25nZXIsIGhlbGxvMik7Cit9CisKK1RF U1QoV1RGLCBEeW5hbWljU3RhdGljU3RyaW5nSW1wbCkKK3sKKyAgICAvLyBDb25zdHJ1Y3QgdXNp bmcgTUFLRV9TVEFUSUNfU1RSSU5HX0lNUEwuCisgICAgU3RyaW5nIGhlbGxvID0gU3RyaW5nSW1w bDo6Y3JlYXRlU3RhdGljU3RyaW5nSW1wbCgiaGVsbG8iLCA1KTsKKyAgICBTdHJpbmcgd29ybGQg PSBTdHJpbmdJbXBsOjpjcmVhdGVTdGF0aWNTdHJpbmdJbXBsKCJ3b3JsZCIsIDUpOworICAgIFN0 cmluZyBsb25nZXIgPSBTdHJpbmdJbXBsOjpjcmVhdGVTdGF0aWNTdHJpbmdJbXBsKCJsb25nZXIi LCA2KTsKKyAgICBTdHJpbmcgaGVsbG8yID0gU3RyaW5nSW1wbDo6Y3JlYXRlU3RhdGljU3RyaW5n SW1wbCgiaGVsbG8iLCA1KTsKKworICAgIGRvU3RhdGljU3RyaW5nSW1wbFRlc3RzKFN0YXRpY1N0 cmluZ0ltcGxUZXN0U2V0OjpEeW5hbWljYWxseUFsbG9jYXRlZEltcGwsIGhlbGxvLCB3b3JsZCwg bG9uZ2VyLCBoZWxsbzIpOwogfQogCiBzdGF0aWMgU3ltYm9sSW1wbDo6U3RhdGljU3ltYm9sSW1w bCBzdGF0aWNTeW1ib2wgeyJDb2NvYSJ9Owo=