Bug 44080 – String find/reverseFind methods need tidying up

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Gavin Barraclough]

These methods have a couple of problems with their interface, and implementation.

These methods take and int index, and return an int – however this is problematic since on 64-bit string indices may have a full 32-bit range.  This spills out into surrounding code, which unsafely casts string indices from unsigned to int.  Code checking the result of these methods check for a mix of "== -1", "< 0", and "== notFound".  Clean this up by changing these methods to take an unsigned starting index, and return a size_t. with a failed match indicated by notFound.  reverseFind also has a special meaning for the starting index argument, in that a negative index is interpreted as an offset back from the end of the string.  Remove this functionality, in the (1!) case where it is used we should just calculate the offset by subtracting from the string's length.

The implementation has a few problems too.  The code is not in webkit style, in using assorted abbreviations in variable names, and implementations of similar find methods with differing argument types were unnecessarily inconsistent.  When find is passed const char* data the string would be handled as latin1 (zero extended to UTF-16) for all characters but the first; this is sign extended.  Case-insensitive find is broken for unicode strings; the hashing optimization is not unicode safe, and could result in false negatives.

Unify UString find methods to match String.

[Gavin Barraclough]

Created attachment 64531 [details]
The patch

[WebKit Review Bot]

Attachment 64531 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/wtf/text/StringImpl.h:304:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
JavaScriptCore/wtf/text/StringImpl.cpp:616:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/network/HTTPParsers.cpp:249:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/css/CSSStyleSelector.cpp:2127:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/LinkHash.cpp:99:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/LinkHash.cpp:100:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 6 in 56 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 64531 [details]
The patch

My only comment is that I think findCaseInsensitive should be renamed findIgnoringCase to match equalIgnoringCase (same for the reverse find case). Otherwise, r=me.

[Gavin Barraclough]

fixed in r65468