Bug 101257 – Add replaceWithLiteral() method to WTF::String

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Chris Dumez]

It would be good to add a replaceLiteral() method to WTFString that takes the replacement string as a literal.
This would avoid uselessly constructing a string from the literal.

[Benjamin Poulain]

"replaceWithLiteral()" maybe?

[Chris Dumez]

Right, this is indeed better, thanks.

[Chris Dumez]

Created attachment 172406 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/te..." exit_code: 1
Source/WTF/wtf/text/WTFString.h:311:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 1 in 4 files


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

[Benjamin Poulain]

Comment on attachment 172406 [details]
Patch

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

Would it be possible to generalize StringImpl::replace(UChar pattern, StringImpl* replacement) so it becomes a special case of this?

> Source/WTF/wtf/text/StringImpl.cpp:1489
> +    if (!replacement)
> +        return this;

This should be !repStrLength.
We kind of have the policy of not reading the char pointer if the length is null. This sometime avoids accidentally using a bad pointer.

[Chris Dumez]

Created attachment 172413 [details]
Patch

Take Benjamin's feedback into consideration.

[Chris Dumez]

Created attachment 172416 [details]
Patch

Rebase on master.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/te..." exit_code: 1
Source/WTF/wtf/text/WTFString.h:311:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 1 in 4 files


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

[Geoffrey Garen]

Comment on attachment 172416 [details]
Patch

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

>> Source/WTF/wtf/text/WTFString.h:311
>> +    ALWAYS_INLINE String& replaceWithLiteral(UChar a, const char (&characters)[charactersCount]) { if (m_impl) m_impl = m_impl->replace(a, characters, charactersCount - 1); return *this; }
> 
> More than one command on the same line in if  [whitespace/parens] [4]

Can we just call this "replace", like the others? The type should be sufficient to distinguish the usage, no?

[Chris Dumez]

(In reply to comment #9)
> (From update of attachment 172416 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172416&action=review
> 
> >> Source/WTF/wtf/text/WTFString.h:311
> >> +    ALWAYS_INLINE String& replaceWithLiteral(UChar a, const char (&characters)[charactersCount]) { if (m_impl) m_impl = m_impl->replace(a, characters, charactersCount - 1); return *this; }
> > 
> > More than one command on the same line in if  [whitespace/parens] [4]
> 
> Can we just call this "replace", like the others? The type should be sufficient to distinguish the usage, no?

Yes, I believe we can call it replace(). I just thought replaceWithLiteral() would be preferable based on StringBuilder::appendLiteral() name.

[Chris Dumez]

Created attachment 172427 [details]
Patch

Take Geoffrey Garen's feedback into consideration.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/te..." exit_code: 1
Source/WTF/wtf/text/WTFString.h:311:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 1 in 4 files


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

[Geoffrey Garen]

There may be ambiguity -- at least in some compilers -- between const char pointer and const char array. In this test program:

void test(const char*)
{
	printf("1\n");
}

void test(char*)
{
	printf("2\n");
}

template<unsigned N>
void test(const char (&)[N])
{
	printf("3\n");
}

int main(int argc, char** argv)
{
	test("hello world");
}

clang printed:

scratch> clang++ -o scratch scratch.cc && ./scratch
1

So it called the const char* function instead of the const char (&)[N] function. :(

Maybe that's why Ben used a special name in StringBuilder.

[Chris Dumez]

(In reply to comment #13)
> There may be ambiguity -- at least in some compilers -- between const char pointer and const char array. In this test program:
> 
> void test(const char*)
> {
>     printf("1\n");
> }
> 
> void test(char*)
> {
>     printf("2\n");
> }
> 
> template<unsigned N>
> void test(const char (&)[N])
> {
>     printf("3\n");
> }
> 
> int main(int argc, char** argv)
> {
>     test("hello world");
> }
> 
> clang printed:
> 
> scratch> clang++ -o scratch scratch.cc && ./scratch
> 1
> 
> So it called the const char* function instead of the const char (&)[N] function. :(
> 
> Maybe that's why Ben used a special name in StringBuilder.

Interesting. I get the same behavior with gcc actually.

Note however, that there is less ambiguity for String::replace() because there is currently no String::replace(UChar a, const char* b) method.

There is however a StringBuilder::append(const char* characters) so this may explain the StringBuilder::appendLiteral() indeed.

[Benjamin Poulain]

(In reply to comment #13)
> So it called the const char* function instead of the const char (&)[N] function. :(
> 
> Maybe that's why Ben used a special name in StringBuilder.

Yep, there is ambiguity, which is why I created "Literal" functions in addition to the regular ones.

In this case, I think I would prefer replaceWithLiteral() than replace() to avoid breaking this code by accident. No hard feeling either way :)

[Benjamin Poulain]

Comment on attachment 172427 [details]
Patch

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

r- for the early return on (!repStrLength). Totally my fault, sorry about that.

Otherwise I think the patch is a great addition. A quick grep tells me it could already be used in a few places in WebKit.
Feel free to change that in the same patch.

> Source/WTF/wtf/text/StringImpl.cpp:1473
> +    if (!repStrLength)
> +        return this;

Thinking about this a bit more, I was wrong before.

If repStrLength is 0, the function should just remove any occurrence of pattern in the source.
Now what I do not understand is why the function returns early if StringImpl is null in the other implementation (although that is unrelated to your patch).

I'd say this is a call for some tests. Please add those cases in the API tests.

> Source/WTF/wtf/text/StringImpl.h:714
> +    WTF_EXPORT_STRING_API PassRefPtr<StringImpl> replace(UChar, const UChar*, unsigned replacementLength);

I am not sure we need this, especially if it is exported.

Do you know of cases where it would be useful to pass a UChar*?

[Chris Dumez]

Comment on attachment 172427 [details]
Patch

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

>> Source/WTF/wtf/text/StringImpl.cpp:1473
>> +        return this;
> 
> Thinking about this a bit more, I was wrong before.
> 
> If repStrLength is 0, the function should just remove any occurrence of pattern in the source.
> Now what I do not understand is why the function returns early if StringImpl is null in the other implementation (although that is unrelated to your patch).
> 
> I'd say this is a call for some tests. Please add those cases in the API tests.

Ok, I will replace repStrLength by replacement in the check and see about adding API tests to Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp.

>> Source/WTF/wtf/text/StringImpl.h:714
>> +    WTF_EXPORT_STRING_API PassRefPtr<StringImpl> replace(UChar, const UChar*, unsigned replacementLength);
> 
> I am not sure we need this, especially if it is exported.
> 
> Do you know of cases where it would be useful to pass a UChar*?

I merely added it for the PassRefPtr<StringImpl> replace(UChar, StringImpl*) refactoring. I would simply not export it if that's OK with you.

>> Source/WTF/wtf/text/WTFString.h:311
>> +    ALWAYS_INLINE String& replace(UChar a, const char (&characters)[charactersCount]) { if (m_impl) m_impl = m_impl->replace(a, characters, charactersCount - 1); return *this; }
> 
> More than one command on the same line in if  [whitespace/parens] [4]

The good thing about calling this function replace() is that there is quite a bit of code calling String::replace(UChar, "xxx") already and they would benefit directly from the new function. However, I don't agree that calling it replace() is not very robust because it will break if someone add a replace(UChar a, const char*) function in the future. I think I would therefore call it replaceWithLiteral() as Benjamin suggested. We can always update the rest of the code afterwards to call this new function.

[Chris Dumez]

Created attachment 172546 [details]
Patch

Take Benjamin's feedback into consideration:
- Rename String::replace() with String::replaceWithString() to be safe
- No longer export "replace(UChar, const UChar*, unsigned replacementLength)"
- Replace repStrLength checks by replacement NULL-checks in new StringImpl::replace() methods
- Add API tests from String::replaceWithString() and corresponding StringImpl method
- Replace String::replace() by String::replaceWithLiteral() where appropriate in the rest of WebKit

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/te..." exit_code: 1
Source/WTF/wtf/text/WTFString.h:311:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 1 in 22 files


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

[Chris Dumez]

Created attachment 172609 [details]
Patch

Update changelogs to match title change from ap.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/te..." exit_code: 1
Source/WTF/wtf/text/WTFString.h:311:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 1 in 22 files


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

[Benjamin Poulain]

Comment on attachment 172609 [details]
Patch

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

Great patch, and I like the tests.

I r- for the replace() taking a UChar*. I would like it to be more like your other replace().

> Source/WTF/wtf/text/StringImpl.cpp:1395
> +    if (!replacement)
> +        return this;

Can you leave that out entirely (and maybe replace it with an assertion)?
Given the two way to use that function are currently:
   -literal
   -the older replace taking a StringImpl
I have the feeling the null "replacement" may be better handled as a null repStrLength.

> Source/WTF/wtf/text/StringImpl.cpp:1434
>              memcpy(data + dstOffset, m_data8 + srcSegmentStart, srcSegmentLength * sizeof(LChar));
>              dstOffset += srcSegmentLength;
> -            memcpy(data + dstOffset, replacement->m_data8, repStrLength * sizeof(LChar));
> +            memcpy(data + dstOffset, replacement, repStrLength * sizeof(LChar));

Just for info: StringImpl has copyChars() that is supposed to be better than memcpy if the input is generic.
If you ever find a place were memcpy is a perf issue in StringImpl::replace(), look at copyChars().

> Source/WTF/wtf/text/StringImpl.cpp:1455
> +        for (unsigned i = 0; i < repStrLength; i++)

Typically ++i in WebKit style.

> Source/WTF/wtf/text/StringImpl.cpp:1473
> +    if (!replacement)
> +        return this;

Ditto regarding replacement.

> Source/WTF/wtf/text/StringImpl.cpp:-1479
> +    while ((srcSegmentEnd = find(pattern, srcSegmentStart)) != notFound) {
> +        srcSegmentLength = srcSegmentEnd - srcSegmentStart;
>          if (srcIs8Bit) {
> -            // Case 3.
>              for (unsigned i = 0; i < srcSegmentLength; i++)
>                  data[i + dstOffset] = m_data8[i + srcSegmentStart];
> -        } else {
> -            // Cases 2 & 4.
> +        } else
>              memcpy(data + dstOffset, m_data16 + srcSegmentStart, srcSegmentLength * sizeof(UChar));
> -        }
> +
>          dstOffset += srcSegmentLength;
> -        if (replacementIs8Bit) {
> -            // Case 4.
> -            for (unsigned i = 0; i < repStrLength; i++)
> -                data[i + dstOffset] = replacement->m_data8[i];
> -        } else {
> -            // Cases 2 & 3.
> -            memcpy(data + dstOffset, replacement->m_data16, repStrLength * sizeof(UChar));
> -        }
> +        memcpy(data + dstOffset, replacement, repStrLength * sizeof(UChar));
> +
>          dstOffset += repStrLength;
>          srcSegmentStart = srcSegmentEnd + 1;
>      }
>  
>      srcSegmentLength = m_length - srcSegmentStart;
>      if (srcIs8Bit) {
> -        // Case 3.
>          for (unsigned i = 0; i < srcSegmentLength; i++)
>              data[i + dstOffset] = m_data8[i + srcSegmentStart];
> -    } else {
> -        // Cases 2 & 4.
> +    } else
>          memcpy(data + dstOffset, m_data16 + srcSegmentStart, srcSegmentLength * sizeof(UChar));
> -    }

You should split this the same way you have done in the other replace, it looks much better. Do is8bit() outside the loop, then to the fallback. If it is not 8bits, do the other loop.

>> Source/WTF/wtf/text/WTFString.h:311
>> +    template<unsigned charactersCount>
>> +    ALWAYS_INLINE String& replaceWithLiteral(UChar a, const char (&characters)[charactersCount]) { if (m_impl) m_impl = m_impl->replace(a, characters, charactersCount - 1); return *this; }
> 
> More than one command on the same line in if  [whitespace/parens] [4]

Please fix the style here. This should really be on multiple lines.

> Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp:79
> +    // Cases for 8Bit source.
> +    testStringImpl = testStringImpl->replace('2', static_cast<const char*>(0), 0);
> +    ASSERT_TRUE(equal(testStringImpl.get(), "1224"));

That's the case I am half convinced about.
Nice you have a test for it!

[Chris Dumez]

Comment on attachment 172609 [details]
Patch

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

>> Source/WTF/wtf/text/StringImpl.cpp:1395
>> +        return this;
> 
> Can you leave that out entirely (and maybe replace it with an assertion)?
> Given the two way to use that function are currently:
>    -literal
>    -the older replace taking a StringImpl
> I have the feeling the null "replacement" may be better handled as a null repStrLength.

Yes, I can replace this with "ASSERT(replacement);" since this is not technically valid input for the method. Note however, that I'll need to remove this case from the API tests then since it will hit the assertion.

>> Source/WTF/wtf/text/StringImpl.cpp:1455
>> +        for (unsigned i = 0; i < repStrLength; i++)
> 
> Typically ++i in WebKit style.

Right, I merely moved the code without editing it but this is worth fixing.

>>> Source/WTF/wtf/text/WTFString.h:311
>>> +    ALWAYS_INLINE String& replaceWithLiteral(UChar a, const char (&characters)[charactersCount]) { if (m_impl) m_impl = m_impl->replace(a, characters, charactersCount - 1); return *this; }
>> 
>> More than one command on the same line in if  [whitespace/parens] [4]
> 
> Please fix the style here. This should really be on multiple lines.

Ok, I was prioritizing consistency with other replace() methods over style but I'll fix it.

>> Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp:79
>> +    ASSERT_TRUE(equal(testStringImpl.get(), "1224"));
> 
> That's the case I am half convinced about.
> Nice you have a test for it!

As explained above, I'll need to remove it since passing a NULL pointer for the replacement argument will now hit the assertion.

[Chris Dumez]

Created attachment 172726 [details]
Patch

Take Benjamin's feedback into consideration.

[Benjamin Poulain]

Comment on attachment 172726 [details]
Patch

Brilliant.

Sorry I made the review by segments, thanks for carefully updating every times.

[WebKit Review Bot]

Comment on attachment 172726 [details]
Patch

Clearing flags on attachment: 172726

Committed r133731: <http://trac.webkit.org/changeset/133731>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.