98299 2012-10-03 14:16:09 -0700 String::remove will convert an 8 bit string to a 16 bit string 2022-02-27 23:24:35 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 msaboff webkit-unassigned benjamin webkit.review.bot oldest_to_newest 734023 0 msaboff 2012-10-03 14:16:09 -0700 The StringImpl::remove() method will up convert the contents of an 8 bit string and the result will always be a 16 bit string. 734028 1 msaboff 2012-10-03 14:19:41 -0700 The remove method is actually in WTFString.cpp (String::remove). 734130 2 166981 msaboff 2012-10-03 16:00:05 -0700 Created attachment 166981 Patch 734263 3 166981 benjamin 2012-10-03 17:43:16 -0700 Comment on attachment 166981 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166981&action=review > Source/WTF/wtf/text/WTFString.cpp:281 > if (static_cast<unsigned>(lengthToRemove) > length() - position) > lengthToRemove = length() - position; No idea if that case is common. If it is, shouldn't we just return a substring sharing impl here? > Source/WTF/wtf/text/WTFString.cpp:289 > + RefPtr<StringImpl> newImpl = StringImpl::createUninitialized(length() - lengthToRemove, data); > + memcpy(data, characters8(), position * sizeof(LChar)); > + memcpy(data + position, characters8() + position + lengthToRemove, > + (length() - lengthToRemove - position) * sizeof(LChar)); > + m_impl = newImpl.release(); I would do an inline template with that code taking CharacterType as argument. 734769 4 msaboff 2012-10-04 09:44:35 -0700 (In reply to comment #3) > (From update of attachment 166981 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166981&action=review > > > Source/WTF/wtf/text/WTFString.cpp:281 > > if (static_cast<unsigned>(lengthToRemove) > length() - position) > > lengthToRemove = length() - position; > > No idea if that case is common. > If it is, shouldn't we just return a substring sharing impl here? From what I've seen, the use of remove is within length range and this wouldn't help. > > Source/WTF/wtf/text/WTFString.cpp:289 > > + RefPtr<StringImpl> newImpl = StringImpl::createUninitialized(length() - lengthToRemove, data); > > + memcpy(data, characters8(), position * sizeof(LChar)); > > + memcpy(data + position, characters8() + position + lengthToRemove, > > + (length() - lengthToRemove - position) * sizeof(LChar)); > > + m_impl = newImpl.release(); > > I would do an inline template with that code taking CharacterType as argument. Done. 734770 5 msaboff 2012-10-04 09:45:17 -0700 Committed r130404: <http://trac.webkit.org/changeset/130404> 166981 2012-10-03 16:00:05 -0700 2022-02-27 23:24:35 -0800 Patch 98299.patch text/plain 1966 msaboff SW5kZXg6IFNvdXJjZS9XVEYvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvQ2hh bmdlTG9nCShyZXZpc2lvbiAxMzAzMzUpCisrKyBTb3VyY2UvV1RGL0NoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDE1IEBACisyMDEyLTEwLTAzICBNaWNoYWVsIFNhYm9mZiAgPG1z YWJvZmZAYXBwbGUuY29tPgorCisgICAgICAgIFN0cmluZzo6cmVtb3ZlIHdpbGwgY29udmVydCBh biA4IGJpdCBzdHJpbmcgdG8gYSAxNiBiaXQgc3RyaW5nCisgICAgICAgIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD05ODI5OQorCisgICAgICAgIFJldmlld2VkIGJ5IE5P Qk9EWSAoT09QUyEpLgorCisgICAgICAgIEFkZGVkIGFuIDggYml0IHBhdGggdG8gcmVtb3ZlKCku CisKKyAgICAgICAgKiB3dGYvdGV4dC9XVEZTdHJpbmcuY3BwOgorICAgICAgICAoV1RGOjpTdHJp bmc6OnJlbW92ZSk6CisKIDIwMTItMTAtMDMgIFl1cnkgU2VtaWtoYXRza3kgIDx5dXJ5c0BjaHJv bWl1bS5vcmc+CiAKICAgICAgICAgUmVtb3ZlIE1lbW9yeUluc3RydW1lbnRhdGlvbjo6YWRkQ29s bGVjdGlvbkVsZW1lbnRzCkluZGV4OiBTb3VyY2UvV1RGL3d0Zi90ZXh0L1dURlN0cmluZy5jcHAK PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PQotLS0gU291cmNlL1dURi93dGYvdGV4dC9XVEZTdHJpbmcuY3BwCShyZXZpc2lv biAxMzAyOTQpCisrKyBTb3VyY2UvV1RGL3d0Zi90ZXh0L1dURlN0cmluZy5jcHAJKHdvcmtpbmcg Y29weSkKQEAgLTI3OSwxMCArMjc5LDIyIEBAIHZvaWQgU3RyaW5nOjpyZW1vdmUodW5zaWduZWQg cG9zaXRpb24sIGkKICAgICAgICAgcmV0dXJuOwogICAgIGlmIChzdGF0aWNfY2FzdDx1bnNpZ25l ZD4obGVuZ3RoVG9SZW1vdmUpID4gbGVuZ3RoKCkgLSBwb3NpdGlvbikKICAgICAgICAgbGVuZ3Ro VG9SZW1vdmUgPSBsZW5ndGgoKSAtIHBvc2l0aW9uOworCisgICAgaWYgKGlzOEJpdCgpKSB7Cisg ICAgICAgIExDaGFyKiBkYXRhOworICAgICAgICBSZWZQdHI8U3RyaW5nSW1wbD4gbmV3SW1wbCA9 IFN0cmluZ0ltcGw6OmNyZWF0ZVVuaW5pdGlhbGl6ZWQobGVuZ3RoKCkgLSBsZW5ndGhUb1JlbW92 ZSwgZGF0YSk7CisgICAgICAgIG1lbWNweShkYXRhLCBjaGFyYWN0ZXJzOCgpLCBwb3NpdGlvbiAq IHNpemVvZihMQ2hhcikpOworICAgICAgICBtZW1jcHkoZGF0YSArIHBvc2l0aW9uLCBjaGFyYWN0 ZXJzOCgpICsgcG9zaXRpb24gKyBsZW5ndGhUb1JlbW92ZSwKKyAgICAgICAgICAgIChsZW5ndGgo KSAtIGxlbmd0aFRvUmVtb3ZlIC0gcG9zaXRpb24pICogc2l6ZW9mKExDaGFyKSk7CisgICAgICAg IG1faW1wbCA9IG5ld0ltcGwucmVsZWFzZSgpOworCisgICAgICAgIHJldHVybjsKKyAgICB9CisK ICAgICBVQ2hhciogZGF0YTsKICAgICBSZWZQdHI8U3RyaW5nSW1wbD4gbmV3SW1wbCA9IFN0cmlu Z0ltcGw6OmNyZWF0ZVVuaW5pdGlhbGl6ZWQobGVuZ3RoKCkgLSBsZW5ndGhUb1JlbW92ZSwgZGF0 YSk7Ci0gICAgbWVtY3B5KGRhdGEsIGNoYXJhY3RlcnMoKSwgcG9zaXRpb24gKiBzaXplb2YoVUNo YXIpKTsKLSAgICBtZW1jcHkoZGF0YSArIHBvc2l0aW9uLCBjaGFyYWN0ZXJzKCkgKyBwb3NpdGlv biArIGxlbmd0aFRvUmVtb3ZlLAorICAgIG1lbWNweShkYXRhLCBjaGFyYWN0ZXJzMTYoKSwgcG9z aXRpb24gKiBzaXplb2YoVUNoYXIpKTsKKyAgICBtZW1jcHkoZGF0YSArIHBvc2l0aW9uLCBjaGFy YWN0ZXJzMTYoKSArIHBvc2l0aW9uICsgbGVuZ3RoVG9SZW1vdmUsCiAgICAgICAgIChsZW5ndGgo KSAtIGxlbmd0aFRvUmVtb3ZlIC0gcG9zaXRpb24pICogc2l6ZW9mKFVDaGFyKSk7CiAgICAgbV9p bXBsID0gbmV3SW1wbC5yZWxlYXNlKCk7CiB9Cg==