74421 2011-12-13 10:45:42 -0800 Cleanup of StringImpl::equal in r102631 post commit 2011-12-13 11:39:26 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 msaboff msaboff darin oldest_to_newest 520175 0 msaboff 2011-12-13 10:45:42 -0800 The following comments were made to https://bugs.webkit.org/show_bug.cgi?id=73622 after it was committed: Comment #4 From Darin Adler 2011-12-13 08:54:19 PST (-) [reply] (From update of attachment 118843 [details]) View in context: https://bugs.webkit.org/attachment.cgi?id=118843&action=review > Source/JavaScriptCore/wtf/text/AtomicString.h:142 > +inline bool operator==(const AtomicString& a, const Vector<UChar>& b) { return a.impl() && equal(static_cast<StringImpl*>(a.impl()), b.data(), b.size()); } Why is the cast needed? > Source/JavaScriptCore/wtf/text/StringImpl.h:566 > +template <typename CharTypeL, typename CharTypeR> > + ALWAYS_INLINE bool equal(const CharTypeL*, const CharTypeR*, unsigned); We don’t normally implement the second line like this. I think that this is too broad an overload for equal. It will allow you to pass any two pointers, including pointers to that aren’t even character types. Instead, I suggest overloading the equal function for all the combinations. Comment #5 From Darin Adler 2011-12-13 08:54:38 PST (-) [reply] (In reply to comment #4) > > Source/JavaScriptCore/wtf/text/StringImpl.h:566 > > +template <typename CharTypeL, typename CharTypeR> > > + ALWAYS_INLINE bool equal(const CharTypeL*, const CharTypeR*, unsigned); > > We don’t normally indent the second line like this. First of all, the cast in the operator==(AtomicString, ...) is needed because the template is too broad. Changing equal to several overloaded functions will address the need for the cast. There is another defect related to building WebKit on Mac Snow Leopard with gcc (https://bugs.webkit.org/show_bug.cgi?id=74387). Changing the equal() templates to overloaded functions might fix this as well. 520181 1 119038 msaboff 2011-12-13 10:49:14 -0800 Created attachment 119038 Patch 520234 2 msaboff 2011-12-13 11:39:26 -0800 Committed r102692: <http://trac.webkit.org/changeset/102692> 119038 2011-12-13 10:49:14 -0800 2011-12-13 11:26:25 -0800 Patch 74421.patch text/plain 7254 msaboff SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTAyNjkwKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE1IEBA CisyMDExLTEyLTEzICBNaWNoYWVsIFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgorCisgICAg ICAgIENsZWFudXAgb2YgU3RyaW5nSW1wbDo6ZXF1YWwgaW4gcjEwMjYzMSBwb3N0IGNvbW1pdAor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NzQ0MjEKKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIHd0Zi90ZXh0 L0F0b21pY1N0cmluZy5oOgorICAgICAgICAoV1RGOjpvcGVyYXRvcj09KTogUmVtb3ZlZCBjYXN0 IG5vIGxvbmdlciBuZWVkZWQuCisgICAgICAgICogd3RmL3RleHQvU3RyaW5nSW1wbC5oOgorICAg ICAgICAoV1RGOjplcXVhbCk6IENoYW5nZWQgdGVtcGxhdGUgdG8gc2V2ZXJhbCBvdmVybG9hZGVk IG1ldGhvZHMuCisKIDIwMTEtMTItMTIgIE1pY2hhZWwgU2Fib2ZmICA8bXNhYm9mZkBhcHBsZS5j b20+CiAKICAgICAgICAgRWxpbWluYXRlIER1cGxpY2F0ZSB3b3JkIGF0IGEgdGltZSBlcXVhbCBj b2RlIGluIFN0cmluZ0ltcGwuY3BwIGFuZCBTdHJpbmdIYXNoLmgKSW5kZXg6IFNvdXJjZS9KYXZh U2NyaXB0Q29yZS93dGYvdGV4dC9BdG9taWNTdHJpbmcuaAo9PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2Uv SmF2YVNjcmlwdENvcmUvd3RmL3RleHQvQXRvbWljU3RyaW5nLmgJKHJldmlzaW9uIDEwMjYzMSkK KysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS93dGYvdGV4dC9BdG9taWNTdHJpbmcuaAkod29ya2lu ZyBjb3B5KQpAQCAtMTM5LDcgKzEzOSw3IEBAIHByaXZhdGU6CiBpbmxpbmUgYm9vbCBvcGVyYXRv cj09KGNvbnN0IEF0b21pY1N0cmluZyYgYSwgY29uc3QgQXRvbWljU3RyaW5nJiBiKSB7IHJldHVy biBhLmltcGwoKSA9PSBiLmltcGwoKTsgfQogYm9vbCBvcGVyYXRvcj09KGNvbnN0IEF0b21pY1N0 cmluZyYsIGNvbnN0IExDaGFyKik7CiBpbmxpbmUgYm9vbCBvcGVyYXRvcj09KGNvbnN0IEF0b21p Y1N0cmluZyYgYSwgY29uc3QgY2hhciogYikgeyByZXR1cm4gV1RGOjplcXVhbChhLmltcGwoKSwg cmVpbnRlcnByZXRfY2FzdDxjb25zdCBMQ2hhcio+KGIpKTsgfQotaW5saW5lIGJvb2wgb3BlcmF0 b3I9PShjb25zdCBBdG9taWNTdHJpbmcmIGEsIGNvbnN0IFZlY3RvcjxVQ2hhcj4mIGIpIHsgcmV0 dXJuIGEuaW1wbCgpICYmIGVxdWFsKHN0YXRpY19jYXN0PFN0cmluZ0ltcGwqPihhLmltcGwoKSks IGIuZGF0YSgpLCBiLnNpemUoKSk7IH0gICAgCitpbmxpbmUgYm9vbCBvcGVyYXRvcj09KGNvbnN0 IEF0b21pY1N0cmluZyYgYSwgY29uc3QgVmVjdG9yPFVDaGFyPiYgYikgeyByZXR1cm4gYS5pbXBs KCkgJiYgZXF1YWwoYS5pbXBsKCksIGIuZGF0YSgpLCBiLnNpemUoKSk7IH0gICAgCiBpbmxpbmUg Ym9vbCBvcGVyYXRvcj09KGNvbnN0IEF0b21pY1N0cmluZyYgYSwgY29uc3QgU3RyaW5nJiBiKSB7 IHJldHVybiBlcXVhbChhLmltcGwoKSwgYi5pbXBsKCkpOyB9CiBpbmxpbmUgYm9vbCBvcGVyYXRv cj09KGNvbnN0IExDaGFyKiBhLCBjb25zdCBBdG9taWNTdHJpbmcmIGIpIHsgcmV0dXJuIGIgPT0g YTsgfQogaW5saW5lIGJvb2wgb3BlcmF0b3I9PShjb25zdCBTdHJpbmcmIGEsIGNvbnN0IEF0b21p Y1N0cmluZyYgYikgeyByZXR1cm4gZXF1YWwoYS5pbXBsKCksIGIuaW1wbCgpKTsgfQpJbmRleDog U291cmNlL0phdmFTY3JpcHRDb3JlL3d0Zi90ZXh0L1N0cmluZ0ltcGwuaAo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvd3RmL3RleHQvU3RyaW5nSW1wbC5oCShyZXZpc2lvbiAx MDI2MzEpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvd3RmL3RleHQvU3RyaW5nSW1wbC5oCSh3 b3JraW5nIGNvcHkpCkBAIC01NjIsOTcgKzU2Miw5MiBAQCBpbmxpbmUgYm9vbCBlcXVhbChjb25z dCBTdHJpbmdJbXBsKiBhLCBjCiBpbmxpbmUgYm9vbCBlcXVhbChjb25zdCBMQ2hhciogYSwgU3Ry aW5nSW1wbCogYikgeyByZXR1cm4gZXF1YWwoYiwgYSk7IH0KIGlubGluZSBib29sIGVxdWFsKGNv bnN0IGNoYXIqIGEsIFN0cmluZ0ltcGwqIGIpIHsgcmV0dXJuIGVxdWFsKGIsIHJlaW50ZXJwcmV0 X2Nhc3Q8Y29uc3QgTENoYXIqPihhKSk7IH0KIGJvb2wgZXF1YWwoY29uc3QgU3RyaW5nSW1wbCos IGNvbnN0IFVDaGFyKiwgdW5zaWduZWQpOwotdGVtcGxhdGUgPHR5cGVuYW1lIENoYXJUeXBlTCwg dHlwZW5hbWUgQ2hhclR5cGVSPgotICAgIEFMV0FZU19JTkxJTkUgYm9vbCBlcXVhbChjb25zdCBD aGFyVHlwZUwqLCBjb25zdCBDaGFyVHlwZVIqLCB1bnNpZ25lZCk7CiAKIC8vIERvIGNvbXBhcmlz b25zIDggb3IgNCBieXRlcy1hdC1hLXRpbWUgb24gYXJjaGl0ZWN0dXJlcyB3aGVyZSBpdCdzIHNh ZmUuCiAjaWYgQ1BVKFg4Nl82NCkKLXRlbXBsYXRlIDw+Ci1BTFdBWVNfSU5MSU5FIGJvb2wgZXF1 YWw8TENoYXIsIExDaGFyPihjb25zdCBMQ2hhciogYSwgY29uc3QgTENoYXIqIGIsIHVuc2lnbmVk IGxlbmd0aCkKK0FMV0FZU19JTkxJTkUgYm9vbCBlcXVhbChjb25zdCBMQ2hhciogYSwgY29uc3Qg TENoYXIqIGIsIHVuc2lnbmVkIGxlbmd0aCkKIHsKICAgICB1bnNpZ25lZCBkd29yZExlbmd0aCA9 IGxlbmd0aCA+PiAzOwotICAgIAorCiAgICAgaWYgKGR3b3JkTGVuZ3RoKSB7CiAgICAgICAgIGNv bnN0IHVpbnQ2NF90KiBhRFdvcmRDaGFyYWN0ZXJzID0gcmVpbnRlcnByZXRfY2FzdDxjb25zdCB1 aW50NjRfdCo+KGEpOwogICAgICAgICBjb25zdCB1aW50NjRfdCogYkRXb3JkQ2hhcmFjdGVycyA9 IHJlaW50ZXJwcmV0X2Nhc3Q8Y29uc3QgdWludDY0X3QqPihiKTsKLSAgICAgICAgCisKICAgICAg ICAgZm9yICh1bnNpZ25lZCBpID0gMDsgaSAhPSBkd29yZExlbmd0aDsgKytpKSB7CiAgICAgICAg ICAgICBpZiAoKmFEV29yZENoYXJhY3RlcnMrKyAhPSAqYkRXb3JkQ2hhcmFjdGVycysrKQogICAg ICAgICAgICAgICAgIHJldHVybiBmYWxzZTsKICAgICAgICAgfQotICAgICAgICAKKwogICAgICAg ICBhID0gcmVpbnRlcnByZXRfY2FzdDxjb25zdCBMQ2hhcio+KGFEV29yZENoYXJhY3RlcnMpOwog ICAgICAgICBiID0gcmVpbnRlcnByZXRfY2FzdDxjb25zdCBMQ2hhcio+KGJEV29yZENoYXJhY3Rl cnMpOwogICAgIH0KLSAgICAKKwogICAgIGlmIChsZW5ndGggJiA0KSB7CiAgICAgICAgIGlmICgq cmVpbnRlcnByZXRfY2FzdDxjb25zdCB1aW50MzJfdCo+KGEpICE9ICpyZWludGVycHJldF9jYXN0 PGNvbnN0IHVpbnQzMl90Kj4oYikpCiAgICAgICAgICAgICByZXR1cm4gZmFsc2U7Ci0gICAgICAg IAorCiAgICAgICAgIGEgKz0gNDsKICAgICAgICAgYiArPSA0OwogICAgIH0KLSAgICAKKwogICAg IGlmIChsZW5ndGggJiAyKSB7CiAgICAgICAgIGlmICgqcmVpbnRlcnByZXRfY2FzdDxjb25zdCB1 aW50MTZfdCo+KGEpICE9ICpyZWludGVycHJldF9jYXN0PGNvbnN0IHVpbnQxNl90Kj4oYikpCiAg ICAgICAgICAgICByZXR1cm4gZmFsc2U7Ci0gICAgICAgIAorCiAgICAgICAgIGEgKz0gMjsKICAg ICAgICAgYiArPSAyOwogICAgIH0KLSAgICAKKwogICAgIGlmIChsZW5ndGggJiAxICYmICgqYSAh PSAqYikpCiAgICAgICAgIHJldHVybiBmYWxzZTsKLSAgICAKKwogICAgIHJldHVybiB0cnVlOwog fQogCi10ZW1wbGF0ZSA8PgotQUxXQVlTX0lOTElORSBib29sIGVxdWFsPFVDaGFyLCBVQ2hhcj4o Y29uc3QgVUNoYXIqIGEsIGNvbnN0IFVDaGFyKiBiLCB1bnNpZ25lZCBsZW5ndGgpCitBTFdBWVNf SU5MSU5FIGJvb2wgZXF1YWwoY29uc3QgVUNoYXIqIGEsIGNvbnN0IFVDaGFyKiBiLCB1bnNpZ25l ZCBsZW5ndGgpCiB7CiAgICAgdW5zaWduZWQgZHdvcmRMZW5ndGggPSBsZW5ndGggPj4gMjsKICAg ICAKICAgICBpZiAoZHdvcmRMZW5ndGgpIHsKICAgICAgICAgY29uc3QgdWludDY0X3QqIGFEV29y ZENoYXJhY3RlcnMgPSByZWludGVycHJldF9jYXN0PGNvbnN0IHVpbnQ2NF90Kj4oYSk7CiAgICAg ICAgIGNvbnN0IHVpbnQ2NF90KiBiRFdvcmRDaGFyYWN0ZXJzID0gcmVpbnRlcnByZXRfY2FzdDxj b25zdCB1aW50NjRfdCo+KGIpOwotICAgICAgICAKKwogICAgICAgICBmb3IgKHVuc2lnbmVkIGkg PSAwOyBpICE9IGR3b3JkTGVuZ3RoOyArK2kpIHsKICAgICAgICAgICAgIGlmICgqYURXb3JkQ2hh cmFjdGVycysrICE9ICpiRFdvcmRDaGFyYWN0ZXJzKyspCiAgICAgICAgICAgICAgICAgcmV0dXJu IGZhbHNlOwogICAgICAgICB9Ci0gICAgICAgIAorCiAgICAgICAgIGEgPSByZWludGVycHJldF9j YXN0PGNvbnN0IFVDaGFyKj4oYURXb3JkQ2hhcmFjdGVycyk7CiAgICAgICAgIGIgPSByZWludGVy cHJldF9jYXN0PGNvbnN0IFVDaGFyKj4oYkRXb3JkQ2hhcmFjdGVycyk7CiAgICAgfQotICAgIAor CiAgICAgaWYgKGxlbmd0aCAmIDIpIHsKICAgICAgICAgaWYgKCpyZWludGVycHJldF9jYXN0PGNv bnN0IHVpbnQzMl90Kj4oYSkgIT0gKnJlaW50ZXJwcmV0X2Nhc3Q8Y29uc3QgdWludDMyX3QqPihi KSkKICAgICAgICAgICAgIHJldHVybiBmYWxzZTsKLSAgICAgICAgCisKICAgICAgICAgYSArPSAy OwogICAgICAgICBiICs9IDI7CiAgICAgfQotICAgIAorCiAgICAgaWYgKGxlbmd0aCAmIDEgJiYg KCphICE9ICpiKSkKICAgICAgICAgcmV0dXJuIGZhbHNlOwotICAgIAorCiAgICAgcmV0dXJuIHRy dWU7CiB9CiAjZWxpZiBDUFUoWDg2KQotdGVtcGxhdGUgPD4KLUFMV0FZU19JTkxJTkUgYm9vbCBl cXVhbDxMQ2hhciwgTENoYXI+KGNvbnN0IExDaGFyKiBhLCBjb25zdCBMQ2hhciogYiwgdW5zaWdu ZWQgbGVuZ3RoKQorQUxXQVlTX0lOTElORSBib29sIGVxdWFsKGNvbnN0IExDaGFyKiBhLCBjb25z dCBMQ2hhciogYiwgdW5zaWduZWQgbGVuZ3RoKQogewogICAgIGNvbnN0IHVpbnQzMl90KiBhQ2hh cmFjdGVycyA9IHJlaW50ZXJwcmV0X2Nhc3Q8Y29uc3QgdWludDMyX3QqPihhKTsKICAgICBjb25z dCB1aW50MzJfdCogYkNoYXJhY3RlcnMgPSByZWludGVycHJldF9jYXN0PGNvbnN0IHVpbnQzMl90 Kj4oYik7Ci0gICAgCisKICAgICB1bnNpZ25lZCB3b3JkTGVuZ3RoID0gbGVuZ3RoID4+IDI7CiAg ICAgZm9yICh1bnNpZ25lZCBpID0gMDsgaSAhPSB3b3JkTGVuZ3RoOyArK2kpIHsKICAgICAgICAg aWYgKCphQ2hhcmFjdGVycysrICE9ICpiQ2hhcmFjdGVycysrKQogICAgICAgICAgICAgcmV0dXJu IGZhbHNlOwogICAgIH0KLSAgICAKKwogICAgIGxlbmd0aCAmPSAzOwotICAgIAorCiAgICAgaWYg KGxlbmd0aCkgewogICAgICAgICBjb25zdCBMQ2hhciogYVJlbWFpbmRlciA9IHJlaW50ZXJwcmV0 X2Nhc3Q8Y29uc3QgTENoYXIqPihhQ2hhcmFjdGVycyk7CiAgICAgICAgIGNvbnN0IExDaGFyKiBi UmVtYWluZGVyID0gcmVpbnRlcnByZXRfY2FzdDxjb25zdCBMQ2hhcio+KGJDaGFyYWN0ZXJzKTsK QEAgLTY2MiwxMiArNjU3LDExIEBAIEFMV0FZU19JTkxJTkUgYm9vbCBlcXVhbDxMQ2hhciwgTENo YXI+KGMKICAgICAgICAgICAgICAgICByZXR1cm4gZmFsc2U7CiAgICAgICAgIH0KICAgICB9Ci0g ICAgCisKICAgICByZXR1cm4gdHJ1ZTsKIH0KIAotdGVtcGxhdGUgPD4KLUFMV0FZU19JTkxJTkUg Ym9vbCBlcXVhbDxVQ2hhciwgVUNoYXI+KGNvbnN0IFVDaGFyKiBhLCBjb25zdCBVQ2hhciogYiwg dW5zaWduZWQgbGVuZ3RoKQorQUxXQVlTX0lOTElORSBib29sIGVxdWFsKGNvbnN0IFVDaGFyKiBh LCBjb25zdCBVQ2hhciogYiwgdW5zaWduZWQgbGVuZ3RoKQogewogICAgIGNvbnN0IHVpbnQzMl90 KiBhQ2hhcmFjdGVycyA9IHJlaW50ZXJwcmV0X2Nhc3Q8Y29uc3QgdWludDMyX3QqPihhKTsKICAg ICBjb25zdCB1aW50MzJfdCogYkNoYXJhY3RlcnMgPSByZWludGVycHJldF9jYXN0PGNvbnN0IHVp bnQzMl90Kj4oYik7CkBAIC02ODMsMTYgKzY3Nyw0NSBAQCBBTFdBWVNfSU5MSU5FIGJvb2wgZXF1 YWw8VUNoYXIsIFVDaGFyPihjCiAgICAgCiAgICAgcmV0dXJuIHRydWU7CiB9CisjZWxzZQorQUxX QVlTX0lOTElORSBib29sIGVxdWFsKGNvbnN0IExDaGFyKiBhLCBjb25zdCBMQ2hhciogYiwgdW5z aWduZWQgbGVuZ3RoKQoreworICAgIGZvciAodW5zaWduZWQgaSA9IDA7IGkgIT0gbGVuZ3RoOyAr K2kpIHsKKyAgICAgICAgaWYgKGFbaV0gIT0gYltpXSkKKyAgICAgICAgICAgIHJldHVybiBmYWxz ZTsKKyAgICB9CisKKyAgICByZXR1cm4gdHJ1ZTsKK30KKworQUxXQVlTX0lOTElORSBib29sIGVx dWFsKGNvbnN0IFVDaGFyKiBhLCBjb25zdCBVQ2hhciogYiwgdW5zaWduZWQgbGVuZ3RoKQorewor ICAgIGZvciAodW5zaWduZWQgaSA9IDA7IGkgIT0gbGVuZ3RoOyArK2kpIHsKKyAgICAgICAgaWYg KGFbaV0gIT0gYltpXSkKKyAgICAgICAgICAgIHJldHVybiBmYWxzZTsKKyAgICB9CisKKyAgICBy ZXR1cm4gdHJ1ZTsKK30KICNlbmRpZgogCi10ZW1wbGF0ZSA8dHlwZW5hbWUgQ2hhclR5cGVMLCB0 eXBlbmFtZSBDaGFyVHlwZVI+Ci1BTFdBWVNfSU5MSU5FIGJvb2wgZXF1YWwoY29uc3QgQ2hhclR5 cGVMKiBhLCBjb25zdCBDaGFyVHlwZVIqIGIsIHVuc2lnbmVkIGxlbmd0aCkKK0FMV0FZU19JTkxJ TkUgYm9vbCBlcXVhbChjb25zdCBMQ2hhciogYSwgY29uc3QgVUNoYXIqIGIsIHVuc2lnbmVkIGxl bmd0aCkKIHsKICAgICBmb3IgKHVuc2lnbmVkIGkgPSAwOyBpICE9IGxlbmd0aDsgKytpKSB7CiAg ICAgICAgIGlmIChhW2ldICE9IGJbaV0pCiAgICAgICAgICAgICByZXR1cm4gZmFsc2U7CiAgICAg fQotICAgIAorCisgICAgcmV0dXJuIHRydWU7Cit9CisKK0FMV0FZU19JTkxJTkUgYm9vbCBlcXVh bChjb25zdCBVQ2hhciogYSwgY29uc3QgTENoYXIqIGIsIHVuc2lnbmVkIGxlbmd0aCkKK3sKKyAg ICBmb3IgKHVuc2lnbmVkIGkgPSAwOyBpICE9IGxlbmd0aDsgKytpKSB7CisgICAgICAgIGlmIChh W2ldICE9IGJbaV0pCisgICAgICAgICAgICByZXR1cm4gZmFsc2U7CisgICAgfQorCiAgICAgcmV0 dXJuIHRydWU7CiB9CiAK