Bug 142772 – Fix StringView misplaced implementations after r181525 and r181558

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

[Dhi Aurrahman]

Fix StringView typos after r181525 and r181558

[Dhi Aurrahman]

Created attachment 248809 [details]
Patch

[Benjamin Poulain]

Comment on attachment 248809 [details]
Patch

I messed up, thanks for fixing after me.

[Darin Adler]

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.

[Benjamin Poulain]

Comment on attachment 248809 [details]
Patch

Oops, I did not notice the test issues :(

[Dhi Aurrahman]

Created attachment 248830 [details]
Patch

[Darin Adler]

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.

[Dhi Aurrahman]

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?

[Dhi Aurrahman]

Created attachment 248907 [details]
Patch

[Dhi Aurrahman]

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

[Darin Adler]

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.

[Dhi Aurrahman]

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.

[Dhi Aurrahman]

Created attachment 248956 [details]
Patch

[Dhi Aurrahman]

Comment on attachment 248956 [details]
Patch

Hope this will work. Thanks to Benjamin for the tip.

[Dhi Aurrahman]

Created attachment 248994 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 248994 [details]
Patch for landing

Clearing flags on attachment: 248994

Committed r181717: <http://trac.webkit.org/changeset/181717>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.