Summary: | Fix StringView misplaced implementations after r181525 and r181558 | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dhi Aurrahman <diorahman> | ||||||||||||
Component: | New Bugs | Assignee: | Dhi Aurrahman <diorahman> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | benjamin, cmarcelo, commit-queue | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Dhi Aurrahman
2015-03-16 21:48:03 PDT
Created attachment 248809 [details]
Patch
Comment on attachment 248809 [details]
Patch
I messed up, thanks for fixing after me.
Comment on attachment 248809 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248809&action=review > Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:220 > + StringView reference(String("abcdefg")); These tests look incorrect. This creates a dangling StringView that points to a String that has already been deleted. I’m surprised that tests written this way don’t immediately fail with the StringView string lifetime assertion when run in a debug build. > Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:230 > + ASSERT_TRUE(reference.startsWith(oneLetterPrefix)); These should be using EXPECT, not ASSERT. Comment on attachment 248809 [details]
Patch
Oops, I did not notice the test issues :(
Created attachment 248830 [details]
Patch
Comment on attachment 248830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248830&action=review No 16-bit coverage in the tests? > Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:228 > + RefPtr<StringImpl> reference = StringImpl::create(reinterpret_cast<const LChar*>("abcdefg")); > + RefPtr<StringImpl> emptyPrefix = StringImpl::create(reinterpret_cast<const LChar*>("")); > + RefPtr<StringImpl> oneLetterPrefix = StringImpl::create(reinterpret_cast<const LChar*>("a")); > + RefPtr<StringImpl> shortPrefix = StringImpl::create(reinterpret_cast<const LChar*>("ab")); > + RefPtr<StringImpl> longPrefix = StringImpl::create(reinterpret_cast<const LChar*>("abcd")); > + RefPtr<StringImpl> upperCasePrefix = StringImpl::create(reinterpret_cast<const LChar*>("AB")); > + RefPtr<StringImpl> notPrefix = StringImpl::create(reinterpret_cast<const LChar*>("cd")); > + RefPtr<StringImpl> withLatin1CharsPrefix = StringImpl::create(reinterpret_cast<const LChar*>("Ã b")); > + RefPtr<StringImpl> spacePrefix = StringImpl::create(reinterpret_cast<const LChar*>(" ")); Should just be: String reference = "abcdefg"; Direct use of StringImpl is unnecessary. > Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:230 > + StringView referenceStringView(*reference.get()); If we are only testing 8-bit then this could just be: StringView referenceStringView(reinterpret_cast<const LChar*>("abcdefg"), 7); We could also make a helper function to make a StringView from a literal to make the test short and readable. Comment on attachment 248830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248830&action=review >> Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:230 >> + StringView referenceStringView(*reference.get()); > > If we are only testing 8-bit then this could just be: > > StringView referenceStringView(reinterpret_cast<const LChar*>("abcdefg"), 7); > > We could also make a helper function to make a StringView from a literal to make the test short and readable. Hi Darin, I'm not sure I completely understand. If I put something like: StringView stringViewFromLiteral(const char* characters) { return StringView(reinterpret_cast<const LChar*>(characters), strlen(characters)); } // I guess this one is for 16-bit StringView stringViewFromUTF8(const char* characters) { return String::fromUTF8(characters); } TEST(WTF, StringViewStartsWithBasic) { StringView reference = stringViewFromLiteral("abcdefg"); StringView referenceUTF8 = stringViewFromLiteral("àîûèô"); StringView prefix = stringViewFromLiteral("abc"); StringView prefixUTF8 = stringViewFromLiteral("àîû"); EXPECT_TRUE(reference.startsWith(prefix)); EXPECT_TRUE(referenceUTF8.startsWith(prefixUTF8)); } Got it pass in debug build. What do you think? Created attachment 248907 [details]
Patch
Comment on attachment 248907 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248907&action=review > Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:491 > + EXPECT_FALSE(referenceUTF8.endsWithIgnoringASCIICase(reference)); I'm not sure about this Comment on attachment 248907 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248907&action=review > Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:221 > +StringView stringViewFromLiteral(const char* characters) > +{ > + return StringView(reinterpret_cast<const LChar*>(characters), strlen(characters)); > +} This is great. > Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:226 > +StringView stringViewFromUTF8(const char* characters) > +{ > + return String::fromUTF8(characters); > +} This won’t work. The string is deleted when the function returns and the StringView is left pointing to deleted memory. Comment on attachment 248907 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248907&action=review >> Tools/TestWebKitAPI/Tests/WTF/StringView.cpp:226 >> +} > > This won’t work. The string is deleted when the function returns and the StringView is left pointing to deleted memory. Honestly, I'm not sure how to create the 16-bit flavor of it. Created attachment 248956 [details]
Patch
Comment on attachment 248956 [details]
Patch
Hope this will work. Thanks to Benjamin for the tip.
Created attachment 248994 [details]
Patch for landing
Comment on attachment 248994 [details] Patch for landing Clearing flags on attachment: 248994 Committed r181717: <http://trac.webkit.org/changeset/181717> All reviewed patches have been landed. Closing bug. |