Add strnstr in wtf/StringExtras.h MacOSX has strnstr(), but at least Linux and Windows don't have it.
Created attachment 38912 [details] Add strnstr for Linux and Windows in StringExtras.h --- 2 files changed, 26 insertions(+), 0 deletions(-)
Comment on attachment 38912 [details] 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.
Comment on attachment 38912 [details] 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!
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).
Created attachment 38917 [details] Add strnstr for Linux and Windows in StringExtras.h --- 2 files changed, 26 insertions(+), 0 deletions(-)
Comment on attachment 38917 [details] 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.
Created attachment 38958 [details] Add strnstr for Linux and Windows in StringExtras.h --- 2 files changed, 26 insertions(+), 0 deletions(-)
(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.
Comment on attachment 38958 [details] Add strnstr for Linux and Windows in StringExtras.h Based partially on Darin's previous r+, this still looks good. Thanks for the patch!
Comment on attachment 38958 [details] Add strnstr for Linux and Windows in StringExtras.h Clearing flags on attachment: 38958 Committed r48012: <http://trac.webkit.org/changeset/48012>
All reviewed patches have been landed. Closing bug.
This seems to be needed for PLATFORM(SOLARIS) also, at least with Solaris 10 sparc.