-
Notifications
You must be signed in to change notification settings - Fork 119
feat(string): Add generic string startsWith/endsWith implementations #1898
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat(string): Add generic string startsWith/endsWith implementations #1898
Conversation
|
|
||
| const size_t strlen = strlen_t(str); | ||
| const size_t smallen = strlen_t(smaller); | ||
| if (strlen < smallen) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<=
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the comment (originally from the AsciiString implementation). The check needs to be < so that startsWith("hello", "hello) == true which is the current behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I read that wrong and original code is also like that. It is good to preserve the original implementation as much as possible because any discrepancy here could potentially cause mismatching in logic.
| } | ||
|
|
||
| inline int strncmp_t(const char *str1, const char *str2, size_t maxcount) { return ::strncmp(str1, str2, maxcount); }; | ||
| inline int strncmp_t(const wchar_t *str1, const wchar_t *str2, size_t maxcount) { return ::wcsncmp(str1, str2, maxcount); }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you measure if this is really faster?
Generally I prefer to have same code paths on all platforms, so that there are less potential discrepancies in behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with comparing 50 char strings.
When both strings were random, the strnicmp_t in this file was about 1/3 faster. Typically ~2100 vs ~3200 microseconds for 100000 iterations.
When testing the worst case, comparing one string with itself, the built-in strnicmp was about 2x faster. In the area of ~8500 vs ~13600 microseconds.
I guess it comes down to if we expect strings to match or not :) But I'll remove these specializations for consistency's sake .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the original impls use simd to load multiple bytes in one go.
xezon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good
This PR adds the string functions
startsWith,startsWithNoCase,endsWithandendsWithNoCasetoWWLib/stringex.h. Supports bothchar*andwchar_t*.Replaced existing functions in AsciiString with calls to the corresponding new implementations:
AsciiString::startsWithAsciiString::startsWithNoCaseAsciiString::endsWithAsciiString::endsWithNoCaseAlso added the same functions for UnicodeString.
This was split out from #1594 where the need for a
endsWithNoCaseforwchar_tsurfaced.