[webkit-reviews] review granted: [Bug 28901] strnstr in wtf/StringExtras.h : [Attachment 38917] Add strnstr for Linux and Windows in StringExtras.h

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Fumitoshi Ukai <ukai at chromium.org>'s
request for review:
Bug 28901: strnstr in wtf/StringExtras.h
https://bugs.webkit.org/show_bug.cgi?id=28901

Attachment 38917: Add strnstr for Linux and Windows in StringExtras.h
https://bugs.webkit.org/attachment.cgi?id=38917&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.