| Summary: | StringView operator==(char*) should check the length of the string | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | youenn fablet <youennf> |
| Component: | New Bugs | Assignee: | youenn fablet <youennf> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | alecflett, beidson, benjamin, cdumez, cmarcelo, commit-queue, darin, dbates, ews-watchlist, jsbell, rniwa, saam, webkit-bug-importer, zan |
| Priority: | P2 | Keywords: | InRadar |
| Version: | WebKit Nightly Build | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Bug Depends on: | |||
| Bug Blocks: | 186761 | ||
| Attachments: | |||
|
Description
youenn fablet
2018-07-06 16:31:37 PDT
Created attachment 344482 [details]
Patch
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.
Created attachment 344490 [details]
Patch
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.
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 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
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 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
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 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
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 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
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 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
Created attachment 344515 [details]
Patch
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.
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? 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? (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. What does String do here? 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.
Created attachment 344611 [details]
Patch
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. 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.
Comment on attachment 344611 [details] Patch Clearing flags on attachment: 344611 Committed r233660: <https://trac.webkit.org/changeset/233660> All reviewed patches have been landed. Closing bug. 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 |