28901 2009-09-01 20:20:54 -0700 strnstr in wtf/StringExtras.h 2010-02-10 18:24:34 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 ukai webkit-unassigned ap eric oinktomi oldest_to_newest 144104 0 ukai 2009-09-01 20:20:54 -0700 Add strnstr in wtf/StringExtras.h MacOSX has strnstr(), but at least Linux and Windows don't have it. 144117 1 38912 ukai 2009-09-01 22:28:02 -0700 Created attachment 38912 Add strnstr for Linux and Windows in StringExtras.h --- 2 files changed, 26 insertions(+), 0 deletions(-) 144118 2 38912 ap 2009-09-01 23:46:18 -0700 Comment on attachment 38912 Add strnstr for Linux and Windows in StringExtras.h > + return NULL; Should be 0, not NULL. Is this new code, or did you take it from somewhere? The implementation in <http://www.opensource.apple.com/source/Libc/Libc-583/string/FreeBSD/strnstr.c> looks quite different, but I do not know if any of the differences are practical. 144128 3 38912 eric 2009-09-02 00:30:08 -0700 Comment on attachment 38912 Add strnstr for Linux and Windows in StringExtras.h I would have used longer names for the variables. Just because this is a POSIX method doesn't mean we have to use old c-style naming conventions. ;) r-, but not for the naming nit, rather for the nits above from ap. :) Thanks for the patch! 144129 4 ukai 2009-09-02 00:34:21 -0700 Thanks for the review (In reply to comment #2) > (From update of attachment 38912 [details]) > > + return NULL; > > Should be 0, not NULL. > > Is this new code, or did you take it from somewhere? The implementation in > <http://www.opensource.apple.com/source/Libc/Libc-583/string/FreeBSD/strnstr.c> > looks quite different, but I do not know if any of the differences are > practical. This is new code. Should we use FreeBSD implementation? I'm slightly worried about copyright issue (FreeBSD one has advertise clause). 144159 5 38917 ukai 2009-09-02 02:21:50 -0700 Created attachment 38917 Add strnstr for Linux and Windows in StringExtras.h --- 2 files changed, 26 insertions(+), 0 deletions(-) 144230 6 38917 darin 2009-09-02 09:18:55 -0700 Comment on attachment 38917 Add strnstr for Linux and Windows in StringExtras.h This is a fairly long function. Do we really want it as an inline? I suppose we don't have a StringExtras.cpp so this is OK for now. The name "find" is not really good as a name for the second argument, because it's a verb, not a noun. The second argument is the string to find, not "a find". So I suggest calling it something like "target". > + int findLength = strlen(find); The result of strlen is a size_t, not an int. There's no reason to use int instead of size_t here. > + for (const char* start = buffer; *start && start + findLength <= buffer + bufferLength; start++) { > + if (*start == *find && strncmp(start, find, findLength) == 0) > + return const_cast<char*>(start); > + } I'd think you'd want to call strncmp on start + 1 and find + 1 and findLength - 1 instead. r=me but I think it's worth fixing some of those things I mentioned. So you could clear the review flag if you intend to post a new patch. 144360 7 38958 ukai 2009-09-02 18:28:14 -0700 Created attachment 38958 Add strnstr for Linux and Windows in StringExtras.h --- 2 files changed, 26 insertions(+), 0 deletions(-) 144361 8 ukai 2009-09-02 18:29:24 -0700 (In reply to comment #6) > (From update of attachment 38917 [details]) > This is a fairly long function. Do we really want it as an inline? I suppose we > don't have a StringExtras.cpp so this is OK for now. > > The name "find" is not really good as a name for the second argument, because > it's a verb, not a noun. The second argument is the string to find, not "a > find". So I suggest calling it something like "target". > > > + int findLength = strlen(find); > > The result of strlen is a size_t, not an int. There's no reason to use int > instead of size_t here. > > > + for (const char* start = buffer; *start && start + findLength <= buffer + bufferLength; start++) { > > + if (*start == *find && strncmp(start, find, findLength) == 0) > > + return const_cast<char*>(start); > > + } > > I'd think you'd want to call strncmp on start + 1 and find + 1 and findLength - > 1 instead. > > r=me but I think it's worth fixing some of those things I mentioned. So you > could clear the review flag if you intend to post a new patch. Thanks for review. Fixed those things. 144438 9 38958 eric 2009-09-03 01:49:31 -0700 Comment on attachment 38958 Add strnstr for Linux and Windows in StringExtras.h Based partially on Darin's previous r+, this still looks good. Thanks for the patch! 144466 10 38958 eric 2009-09-03 02:38:07 -0700 Comment on attachment 38958 Add strnstr for Linux and Windows in StringExtras.h Clearing flags on attachment: 38958 Committed r48012: <http://trac.webkit.org/changeset/48012> 144467 11 eric 2009-09-03 02:38:11 -0700 All reviewed patches have been landed. Closing bug. 189564 12 oinktomi 2010-02-10 18:24:34 -0800 This seems to be needed for PLATFORM(SOLARIS) also, at least with Solaris 10 sparc. 38912 2009-09-01 22:28:02 -0700 2009-09-02 02:21:45 -0700 Add strnstr for Linux and Windows in StringExtras.h bug-28901-20090902142800.patch text/plain 1246 ukai ZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZyBiL0phdmFTY3JpcHRDb3JlL0No YW5nZUxvZwppbmRleCBlZjEyMWZhLi5hMGMwOGE0IDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29y ZS9DaGFuZ2VMb2cKKysrIGIvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTMg QEAKKzIwMDktMDktMDEgIEZ1bWl0b3NoaSBVa2FpICA8dWthaUBjaHJvbWl1bS5vcmc+CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQWRkIHN0cm5zdHIg Zm9yIExpbnV4IGFuZCBXaW5kb3dzIGluIFN0cmluZ0V4dHJhcy5oCisgICAgICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yODkwMQorCisgICAgICAgICogd3RmL1N0 cmluZ0V4dHJhcy5oOgorICAgICAgICAoc3RybnN0cik6CisKIDIwMDktMDgtMzEgIFJvYmVydCBB Z29zdG9uICA8QWdvc3Rvbi5Sb2JlcnRAc3R1ZC51LXN6ZWdlZC5odT4KIAogICAgICAgICBSZXZp ZXdlZCBieSBHYXZpbiBCYXJyYWNsb3VnaC4KZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL3d0 Zi9TdHJpbmdFeHRyYXMuaCBiL0phdmFTY3JpcHRDb3JlL3d0Zi9TdHJpbmdFeHRyYXMuaAppbmRl eCAxYzIzMzkwLi41MmVkNzYzIDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29yZS93dGYvU3RyaW5n RXh0cmFzLmgKKysrIGIvSmF2YVNjcmlwdENvcmUvd3RmL1N0cmluZ0V4dHJhcy5oCkBAIC04NSw0 ICs4NSwyMCBAQCBpbmxpbmUgaW50IHN0cmNhc2VjbXAoY29uc3QgY2hhciogczEsIGNvbnN0IGNo YXIqIHMyKQogCiAjZW5kaWYKIAorI2lmIFBMQVRGT1JNKFdJTl9PUykgfHwgUExBVEZPUk0oTElO VVgpCisKK2lubGluZSBjaGFyKiBzdHJuc3RyKGNvbnN0IGNoYXIqIHMxLCBjb25zdCBjaGFyKiBz Miwgc2l6ZV90IG4pCit7CisgICAgaW50IHMybGVuID0gc3RybGVuKHMyKTsKKyAgICBpZiAoczJs ZW4gPT0gMCkKKyAgICAgICAgcmV0dXJuIGNvbnN0X2Nhc3Q8Y2hhcio+KHMxKTsKKyAgICBmb3Ig KGNvbnN0IGNoYXIqIHAgPSBzMTsgKnAgJiYgcCArIHMybGVuIDw9IHMxICsgbjsgcCsrKSB7Cisg ICAgICAgIGlmICgqcCA9PSAqczIgJiYgc3RybmNtcChwLCBzMiwgczJsZW4pID09IDApCisgICAg ICAgICAgICByZXR1cm4gY29uc3RfY2FzdDxjaGFyKj4ocCk7CisgICAgfQorICAgIHJldHVybiBO VUxMOworfQorCisjZW5kaWYKKwogI2VuZGlmIC8vIFdURl9TdHJpbmdFeHRyYXNfaA== 38917 2009-09-02 02:21:50 -0700 2009-09-02 18:28:09 -0700 Add strnstr for Linux and Windows in StringExtras.h bug-28901-20090902182148.patch text/plain 1337 ukai ZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZyBiL0phdmFTY3JpcHRDb3JlL0No YW5nZUxvZwppbmRleCBlZjEyMWZhLi42NDQ1NTVmIDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29y ZS9DaGFuZ2VMb2cKKysrIGIvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTMg QEAKKzIwMDktMDktMDIgIEZ1bWl0b3NoaSBVa2FpICA8dWthaUBjaHJvbWl1bS5vcmc+CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQWRkIHN0cm5zdHIg Zm9yIExpbnV4IGFuZCBXaW5kb3dzIGluIFN0cmluZ0V4dHJhcy5oCisgICAgICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yODkwMQorCisgICAgICAgICogd3RmL1N0 cmluZ0V4dHJhcy5oOgorICAgICAgICAoc3RybnN0cik6CisKIDIwMDktMDgtMzEgIFJvYmVydCBB Z29zdG9uICA8QWdvc3Rvbi5Sb2JlcnRAc3R1ZC51LXN6ZWdlZC5odT4KIAogICAgICAgICBSZXZp ZXdlZCBieSBHYXZpbiBCYXJyYWNsb3VnaC4KZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL3d0 Zi9TdHJpbmdFeHRyYXMuaCBiL0phdmFTY3JpcHRDb3JlL3d0Zi9TdHJpbmdFeHRyYXMuaAppbmRl eCAxYzIzMzkwLi5lMzZjMTY2IDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29yZS93dGYvU3RyaW5n RXh0cmFzLmgKKysrIGIvSmF2YVNjcmlwdENvcmUvd3RmL1N0cmluZ0V4dHJhcy5oCkBAIC04NSw0 ICs4NSwyMCBAQCBpbmxpbmUgaW50IHN0cmNhc2VjbXAoY29uc3QgY2hhciogczEsIGNvbnN0IGNo YXIqIHMyKQogCiAjZW5kaWYKIAorI2lmIFBMQVRGT1JNKFdJTl9PUykgfHwgUExBVEZPUk0oTElO VVgpCisKK2lubGluZSBjaGFyKiBzdHJuc3RyKGNvbnN0IGNoYXIqIGJ1ZmZlciwgY29uc3QgY2hh ciogZmluZCwgc2l6ZV90IGJ1ZmZlckxlbmd0aCkKK3sKKyAgICBpbnQgZmluZExlbmd0aCA9IHN0 cmxlbihmaW5kKTsKKyAgICBpZiAoZmluZExlbmd0aCA9PSAwKQorICAgICAgICByZXR1cm4gY29u c3RfY2FzdDxjaGFyKj4oYnVmZmVyKTsKKyAgICBmb3IgKGNvbnN0IGNoYXIqIHN0YXJ0ID0gYnVm ZmVyOyAqc3RhcnQgJiYgc3RhcnQgKyBmaW5kTGVuZ3RoIDw9IGJ1ZmZlciArIGJ1ZmZlckxlbmd0 aDsgc3RhcnQrKykgeworICAgICAgICBpZiAoKnN0YXJ0ID09ICpmaW5kICYmIHN0cm5jbXAoc3Rh cnQsIGZpbmQsIGZpbmRMZW5ndGgpID09IDApCisgICAgICAgICAgICByZXR1cm4gY29uc3RfY2Fz dDxjaGFyKj4oc3RhcnQpOworICAgIH0KKyAgICByZXR1cm4gMDsKK30KKworI2VuZGlmCisKICNl bmRpZiAvLyBXVEZfU3RyaW5nRXh0cmFzX2g= 38958 2009-09-02 18:28:14 -0700 2009-09-03 02:38:07 -0700 Add strnstr for Linux and Windows in StringExtras.h bug-28901-20090903102812.patch text/plain 1380 ukai ZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZyBiL0phdmFTY3JpcHRDb3JlL0No YW5nZUxvZwppbmRleCAwMzY4OGJlLi4wYzk0OTMyIDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29y ZS9DaGFuZ2VMb2cKKysrIGIvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTMg QEAKKzIwMDktMDktMDIgIEZ1bWl0b3NoaSBVa2FpICA8dWthaUBjaHJvbWl1bS5vcmc+CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQWRkIHN0cm5zdHIg Zm9yIExpbnV4IGFuZCBXaW5kb3dzIGluIFN0cmluZ0V4dHJhcy5oCisgICAgICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yODkwMQorCisgICAgICAgICogd3RmL1N0 cmluZ0V4dHJhcy5oOgorICAgICAgICAoc3RybnN0cik6CisKIDIwMDktMDktMDIgIEdhdmluIEJh cnJhY2xvdWdoICA8YmFycmFjbG91Z2hAYXBwbGUuY29tPgogCiAgICAgICAgIFNob3VsZCBjcmFz aCBpZiBKSVQgY29kZSBidWZmZXIgYWxsb2NhdGlvbiBmYWlscy4KZGlmZiAtLWdpdCBhL0phdmFT Y3JpcHRDb3JlL3d0Zi9TdHJpbmdFeHRyYXMuaCBiL0phdmFTY3JpcHRDb3JlL3d0Zi9TdHJpbmdF eHRyYXMuaAppbmRleCAxYzIzMzkwLi41NTllM2YyIDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29y ZS93dGYvU3RyaW5nRXh0cmFzLmgKKysrIGIvSmF2YVNjcmlwdENvcmUvd3RmL1N0cmluZ0V4dHJh cy5oCkBAIC04NSw0ICs4NSwyMCBAQCBpbmxpbmUgaW50IHN0cmNhc2VjbXAoY29uc3QgY2hhciog czEsIGNvbnN0IGNoYXIqIHMyKQogCiAjZW5kaWYKIAorI2lmIFBMQVRGT1JNKFdJTl9PUykgfHwg UExBVEZPUk0oTElOVVgpCisKK2lubGluZSBjaGFyKiBzdHJuc3RyKGNvbnN0IGNoYXIqIGJ1ZmZl ciwgY29uc3QgY2hhciogdGFyZ2V0LCBzaXplX3QgYnVmZmVyTGVuZ3RoKQoreworICAgIHNpemVf dCB0YXJnZXRMZW5ndGggPSBzdHJsZW4odGFyZ2V0KTsKKyAgICBpZiAodGFyZ2V0TGVuZ3RoID09 IDApCisgICAgICAgIHJldHVybiBjb25zdF9jYXN0PGNoYXIqPihidWZmZXIpOworICAgIGZvciAo Y29uc3QgY2hhciogc3RhcnQgPSBidWZmZXI7ICpzdGFydCAmJiBzdGFydCArIHRhcmdldExlbmd0 aCA8PSBidWZmZXIgKyBidWZmZXJMZW5ndGg7IHN0YXJ0KyspIHsKKyAgICAgICAgaWYgKCpzdGFy dCA9PSAqdGFyZ2V0ICYmIHN0cm5jbXAoc3RhcnQgKyAxLCB0YXJnZXQgKyAxLCB0YXJnZXRMZW5n dGggLSAxKSA9PSAwKQorICAgICAgICAgICAgcmV0dXJuIGNvbnN0X2Nhc3Q8Y2hhcio+KHN0YXJ0 KTsKKyAgICB9CisgICAgcmV0dXJuIDA7Cit9CisKKyNlbmRpZgorCiAjZW5kaWYgLy8gV1RGX1N0 cmluZ0V4dHJhc19o