Bug 187422 – StringView operator==(char*) should check the length of the string

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[youenn fablet]

StringView operator==(char*) should check the length of the string

[youenn fablet]

Created attachment 344482 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:305:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:306:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:307:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
Total errors found: 3 in 4 files


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

[youenn fablet]

Created attachment 344490 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:305:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:306:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:307:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
Total errors found: 3 in 6 files


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

[EWS Watchlist]

Comment on attachment 344490 [details]
Patch

Attachment 344490 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8462338

New failing tests:
imported/w3c/web-platform-tests/eventsource/format-field-parsing.htm

[EWS Watchlist]

Created attachment 344496 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 344490 [details]
Patch

Attachment 344490 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8462574

New failing tests:
imported/w3c/web-platform-tests/eventsource/format-field-parsing.htm
media/video-fullscreen-reload-crash.html

[EWS Watchlist]

Created attachment 344498 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 344490 [details]
Patch

Attachment 344490 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8462495

New failing tests:
imported/w3c/web-platform-tests/eventsource/format-field-parsing.htm

[EWS Watchlist]

Created attachment 344501 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 344490 [details]
Patch

Attachment 344490 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8462694

New failing tests:
imported/w3c/web-platform-tests/eventsource/format-field-parsing.htm

[EWS Watchlist]

Created attachment 344504 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 344490 [details]
Patch

Attachment 344490 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8463204

New failing tests:
imported/w3c/web-platform-tests/eventsource/format-field-parsing.htm

[EWS Watchlist]

Created attachment 344510 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6

[youenn fablet]

Created attachment 344515 [details]
Patch

[EWS Watchlist]

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


ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:305:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:306:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:307:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:311:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:312:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
Total errors found: 5 in 9 files


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

[Chris Dumez]

Comment on attachment 344515 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344515&action=review

> Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:312
> +    EXPECT_FALSE(a == "Hell");

Is this really the expected behavior, I would have personally expected this to be TRUE. Wouldn't strcmp() be a match here?

[Saam Barati]

Comment on attachment 344515 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344515&action=review

> Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:310
> +    a = StringView { (const LChar*)test, 5 };

static_cast

>> Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:312
>> +    EXPECT_FALSE(a == "Hell");
> 
> Is this really the expected behavior, I would have personally expected this to be TRUE. Wouldn't strcmp() be a match here?

Why? Isn't the point here that our strings are allowed to have the null byte in them and they are meaningful bytes?

[youenn fablet]

(In reply to Saam Barati from comment #18)
> Comment on attachment 344515 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344515&action=review
> 
> > Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:310
> > +    a = StringView { (const LChar*)test, 5 };
> 
> static_cast
> 
> >> Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:312
> >> +    EXPECT_FALSE(a == "Hell");
> > 
> > Is this really the expected behavior, I would have personally expected this to be TRUE. Wouldn't strcmp() be a match here?
> 
> Why? Isn't the point here that our strings are allowed to have the null byte
> in them and they are meaningful bytes?

That was also my reasoning.

It is true that there is some inconsistency with StringView case insensitive comparison.
In that case, strlen() is used for any const char* pointer.
That would mean that for "hell\0o", case insensitive check would check the first 4 bytes while for this patch, we would try to check the 6 first bytes.

Also, if we were to add back a StringView operator==(char*), we would probably use strlen() and a comparison of the same pointer depending on its type might end up with different results.

Maybe we should add an ASSERT there, something like ASSERT(length == strlen(b))
Or just use strlen() everywhere.

[Saam Barati]

What does String do here?

[youenn fablet]

auto test1 = "Hell\0";
a = StringView { (const LChar*)test1, 5 };

String test2 = a.toString();
EXPECT_FALSE(test2 == "Hell\0");
EXPECT_FALSE(test2 == "Hell");

String test3 = "Hello";
EXPECT_TRUE(test3 == "Hello\0");
EXPECT_TRUE(test3 == "Hello");

I believe String is doing as if using strlen(). It seems safer to align.

[youenn fablet]

Created attachment 344611 [details]
Patch

[youenn fablet]

I went the strlen() easy approach.
I guess we could try to optimize it here a bit but this seems more urgent to fix the correctness here.

[EWS Watchlist]

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


ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:305:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:306:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:307:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:311:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:312:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:315:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:316:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
Total errors found: 7 in 9 files


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

[WebKit Commit Bot]

Comment on attachment 344611 [details]
Patch

Clearing flags on attachment: 344611

Committed r233660: <https://trac.webkit.org/changeset/233660>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41993422>

[Zan Dobersek]

Comment on attachment 344611 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344611&action=review

> Source/WTF/wtf/linux/MemoryFootprintLinux.cpp:75
> -                isAnonymous = pathString == "[heap]"_s || pathString.startsWith("[stack");
> +                isAnonymous = pathString == "[heap]" | pathString.startsWith("[stack");

This OR operation was I guess accidentally made bitwise. Reverted in r233677.
https://trac.webkit.org/changeset/233677/webkit