StringImpl::reverseFind() has code to handle single character matches optimally if both the subject and match strings are the same bit-ness. The code should be optimized for all four bit ness combinations.
Created attachment 168778 [details] Patch
Comment on attachment 168778 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168778&action=review > Source/WTF/wtf/text/StringImpl.cpp:1145 > + return WTF::reverseFind(characters(), ourLength, matchCharacter, index); characters() -> characters16()
(In reply to comment #2) > (From update of attachment 168778 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=168778&action=review > > > Source/WTF/wtf/text/StringImpl.cpp:1145 > > + return WTF::reverseFind(characters(), ourLength, matchCharacter, index); > > characters() -> characters16() Done in my sandbox, want another patch?
> > characters() -> characters16() > > Done in my sandbox, want another patch? That's ok, that was just a quick comment before going out. I'll have a look at your patch.
Comment on attachment 168778 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168778&action=review > Source/WTF/wtf/text/StringImpl.cpp:1035 > + UChar matchCharacter = matchString->is8Bit() ? matchString->characters8()[0] : matchString->characters16()[0]; StringImpl's operator[] does that for you. > Source/WTF/wtf/text/StringImpl.cpp:-1135 > - if (is8Bit() && matchString->is8Bit()) > - return WTF::reverseFind(characters8(), ourLength, matchString->characters8()[0], index); > - return WTF::reverseFind(characters(), ourLength, matchString->characters()[0], index); This was intentional as the reverseFind(LChar*, UChar) was not implemented. Then I forgot about it. Does WTF::reverseFind() has this combination now? I don't see it in StringImpl.h. > Source/WTF/wtf/text/StringImpl.cpp:1141 > + UChar matchCharacter = matchString->is8Bit() ? matchString->characters8()[0] : matchString->characters16()[0]; see StringImpl's operator[].
Created attachment 168832 [details] Patch with review updates (In reply to comment #5) > (From update of attachment 168778 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=168778&action=review > > > Source/WTF/wtf/text/StringImpl.cpp:1035 > > + UChar matchCharacter = matchString->is8Bit() ? matchString->characters8()[0] : matchString->characters16()[0]; > > StringImpl's operator[] does that for you. Fixed. I think I added operator[]. > > Source/WTF/wtf/text/StringImpl.cpp:-1135 > > - if (is8Bit() && matchString->is8Bit()) > > - return WTF::reverseFind(characters8(), ourLength, matchString->characters8()[0], index); > > - return WTF::reverseFind(characters(), ourLength, matchString->characters()[0], index); > > This was intentional as the reverseFind(LChar*, UChar) was not implemented. Then I forgot about it. > > Does WTF::reverseFind() has this combination now? I don't see it in StringImpl.h. I updated reverseFind() to be like find(). > > Source/WTF/wtf/text/StringImpl.cpp:1141 > > + UChar matchCharacter = matchString->is8Bit() ? matchString->characters8()[0] : matchString->characters16()[0]; > > see StringImpl's operator[].
Comment on attachment 168832 [details] Patch with review updates View in context: https://bugs.webkit.org/attachment.cgi?id=168832&action=review > Source/WTF/wtf/text/StringImpl.h:1015 > +ALWAYS_INLINE size_t reverseFind(const UChar* characters, unsigned length, LChar matchCharacter, unsigned index = 0) default value of index looks wrong. > Source/WTF/wtf/text/StringImpl.h:1020 > +inline size_t reverseFind(const LChar* characters, unsigned length, UChar matchCharacter, unsigned index = 0) Ditto.
Comment on attachment 168832 [details] Patch with review updates View in context: https://bugs.webkit.org/attachment.cgi?id=168832&action=review > Source/WTF/wtf/text/StringImpl.h:1022 > + if (matchCharacter & ~0xFF) Not happy with the magic math here. There’s a reason we have a named function isASCII for the case like this with 0x7F, and I’d prefer a named functin for this kind of use too.
Committed r131524: <http://trac.webkit.org/changeset/131524>