Bug 28901 - strnstr in wtf/StringExtras.h
Summary: strnstr in wtf/StringExtras.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-01 20:20 PDT by Fumitoshi Ukai
Modified: 2010-02-10 18:24 PST (History)
3 users (show)

See Also:


Attachments
Add strnstr for Linux and Windows in StringExtras.h (1.22 KB, patch)
2009-09-01 22:28 PDT, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Add strnstr for Linux and Windows in StringExtras.h (1.31 KB, patch)
2009-09-02 02:21 PDT, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Add strnstr for Linux and Windows in StringExtras.h (1.35 KB, patch)
2009-09-02 18:28 PDT, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fumitoshi Ukai 2009-09-01 20:20:54 PDT
Add strnstr in wtf/StringExtras.h
MacOSX has strnstr(), but at least Linux and Windows don't have it.
Comment 1 Fumitoshi Ukai 2009-09-01 22:28:02 PDT
Created attachment 38912 [details]
Add strnstr for Linux and Windows in StringExtras.h


---
 2 files changed, 26 insertions(+), 0 deletions(-)
Comment 2 Alexey Proskuryakov 2009-09-01 23:46:18 PDT
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 3 Eric Seidel (no email) 2009-09-02 00:30:08 PDT
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!
Comment 4 Fumitoshi Ukai 2009-09-02 00:34:21 PDT
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).
Comment 5 Fumitoshi Ukai 2009-09-02 02:21:50 PDT
Created attachment 38917 [details]
Add strnstr for Linux and Windows in StringExtras.h


---
 2 files changed, 26 insertions(+), 0 deletions(-)
Comment 6 Darin Adler 2009-09-02 09:18:55 PDT
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.
Comment 7 Fumitoshi Ukai 2009-09-02 18:28:14 PDT
Created attachment 38958 [details]
Add strnstr for Linux and Windows in StringExtras.h


---
 2 files changed, 26 insertions(+), 0 deletions(-)
Comment 8 Fumitoshi Ukai 2009-09-02 18:29:24 PDT
(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 9 Eric Seidel (no email) 2009-09-03 01:49:31 PDT
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 10 Eric Seidel (no email) 2009-09-03 02:38:07 PDT
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>
Comment 11 Eric Seidel (no email) 2009-09-03 02:38:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Noah Friedman 2010-02-10 18:24:34 PST
This seems to be needed for PLATFORM(SOLARIS) also, at least with Solaris 10 sparc.