Bug 28901 – strnstr in wtf/StringExtras.h

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Fumitoshi Ukai]

Add strnstr in wtf/StringExtras.h
MacOSX has strnstr(), but at least Linux and Windows don't have it.

[Fumitoshi Ukai]

Created attachment 38912 [details]
Add strnstr for Linux and Windows in StringExtras.h


---
 2 files changed, 26 insertions(+), 0 deletions(-)

[Alexey Proskuryakov]

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.

[Eric Seidel (no email)]

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!

[Fumitoshi Ukai]

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).

[Fumitoshi Ukai]

Created attachment 38917 [details]
Add strnstr for Linux and Windows in StringExtras.h


---
 2 files changed, 26 insertions(+), 0 deletions(-)

[Darin Adler]

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.

[Fumitoshi Ukai]

Created attachment 38958 [details]
Add strnstr for Linux and Windows in StringExtras.h


---
 2 files changed, 26 insertions(+), 0 deletions(-)

[Fumitoshi Ukai]

(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.

[Eric Seidel (no email)]

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!

[Eric Seidel (no email)]

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>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Noah Friedman]

This seems to be needed for PLATFORM(SOLARIS) also, at least with Solaris 10 sparc.