Bug 101257

"replaceWithLiteral()" maybe?

Right, this is indeed better, thanks.

Created attachment 172406 [details] Patch

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.

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.

Created attachment 172413 [details] Patch Take Benjamin's feedback into consideration.

Created attachment 172416 [details] Patch Rebase on master.

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.

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?

(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.

Created attachment 172427 [details] Patch Take Geoffrey Garen's feedback into consideration.

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.

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.

(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.

(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 :)

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*?

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.

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

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.

Created attachment 172609 [details] Patch Update changelogs to match title change from ap.

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.

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!

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.

Created attachment 172726 [details] Patch Take Benjamin's feedback into consideration.

Comment on attachment 172726 [details] Patch Brilliant. Sorry I made the review by segments, thanks for carefully updating every times.

Comment on attachment 172726 [details] Patch Clearing flags on attachment: 172726 Committed r133731: <http://trac.webkit.org/changeset/133731>

All reviewed patches have been landed. Closing bug.