From b444943db27dcc32203837141b4881bfc61930e3 Mon Sep 17 00:00:00 2001 From: John Stiles Date: Mon, 22 Jun 2020 17:54:05 -0400 Subject: [PATCH] Update SkString::resize to preserve string contents efficiently. Without this change, the following unit test failures would occur: StringTest.cpp:425 [String_resize_grow] StringTest.cpp:435 [String_resize_after_assignment] StringTest.cpp:438 [String_resize_after_assignment] StringTest.cpp:444 [String_resize_after_copy_construction] StringTest.cpp:446 [String_resize_after_copy_construction] Change-Id: Ib4f63d51604e55d32f1049136b733ee905b72039 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/298217 Commit-Queue: John Stiles Reviewed-by: Mike Reed Reviewed-by: Ben Wagner Auto-Submit: John Stiles --- include/core/SkString.h | 4 ++-- src/core/SkString.cpp | 39 ++++++++++++++++++++------------- tests/StringTest.cpp | 48 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 17 deletions(-) diff --git a/include/core/SkString.h b/include/core/SkString.h index 357a08ef57..6f1d972da2 100644 --- a/include/core/SkString.h +++ b/include/core/SkString.h @@ -181,10 +181,10 @@ public: char& operator[](size_t n) { return this->writable_str()[n]; } void reset(); - /** Destructive resize, does not preserve contents. + /** String contents are preserved on resize. (For destructive resize, `set(nullptr, length)`.) * `resize` automatically reserves an extra byte at the end of the buffer for a null terminator. */ - void resize(size_t len) { this->set(nullptr, len); } + void resize(size_t len); void set(const SkString& src) { *this = src; } void set(const char text[]); void set(const char text[], size_t len); diff --git a/src/core/SkString.cpp b/src/core/SkString.cpp index 20f65c0cd3..13743b2212 100644 --- a/src/core/SkString.cpp +++ b/src/core/SkString.cpp @@ -42,9 +42,9 @@ static StringBuffer apply_format_string(const char* format, va_list args, char ( } // Our text was too long to fit on the stack! However, we now know how much space we need to - // format it. Format the string into our heap buffer. `resize` automatically reserves an extra + // format it. Format the string into our heap buffer. `set` automatically reserves an extra // byte at the end of the buffer for a null terminator, so we don't need to add one here. - heapBuffer->resize(outLength); + heapBuffer->set(nullptr, outLength); char* heapBufferDest = heapBuffer->writable_str(); SkDEBUGCODE(int checkLength =) std::vsnprintf(heapBufferDest, outLength + 1, format, argsCopy); SkASSERT(checkLength == outLength); @@ -327,31 +327,40 @@ char* SkString::writable_str() { return fRec->data(); } +void SkString::resize(size_t len) { + len = trim_size_t_to_u32(len); + if (0 == len) { + this->reset(); + } else if (fRec->unique() && ((len >> 2) <= (fRec->fLength >> 2))) { + // Use less of the buffer we have without allocating a smaller one. + char* p = this->writable_str(); + p[len] = '\0'; + fRec->fLength = SkToU32(len); + } else { + SkString newString(len); + char* dest = newString.writable_str(); + int copyLen = std::min(len, this->size()); + memcpy(dest, this->c_str(), copyLen); + dest[copyLen] = '\0'; + this->swap(newString); + } +} + void SkString::set(const char text[]) { this->set(text, text ? strlen(text) : 0); } void SkString::set(const char text[], size_t len) { len = trim_size_t_to_u32(len); - bool unique = fRec->unique(); if (0 == len) { this->reset(); - } else if (unique && len <= fRec->fLength) { - // should we resize if len <<<< fLength, to save RAM? (e.g. len < (fLength>>1))? - // just use less of the buffer without allocating a smaller one + } else if (fRec->unique() && ((len >> 2) <= (fRec->fLength >> 2))) { + // Use less of the buffer we have without allocating a smaller one. char* p = this->writable_str(); if (text) { memcpy(p, text, len); } - p[len] = 0; - fRec->fLength = SkToU32(len); - } else if (unique && (fRec->fLength >> 2) == (len >> 2)) { - // we have spare room in the current allocation, so don't alloc a larger one - char* p = this->writable_str(); - if (text) { - memcpy(p, text, len); - } - p[len] = 0; + p[len] = '\0'; fRec->fLength = SkToU32(len); } else { SkString tmp(text, len); diff --git a/tests/StringTest.cpp b/tests/StringTest.cpp index 0a7d375a07..5bd6bb9913 100644 --- a/tests/StringTest.cpp +++ b/tests/StringTest.cpp @@ -403,3 +403,51 @@ DEF_TEST(String_VAList_overflow, r) { test_va_list_overflow_append(r, "%1999s", " "); test_va_list_overflow_prepend(r, "%1999s", " "); } + +DEF_TEST(String_resize_to_nothing, r) { + SkString s("hello world!"); + REPORTER_ASSERT(r, s.equals("hello world!")); + s.resize(0); + REPORTER_ASSERT(r, s.equals("")); +} + +DEF_TEST(String_resize_shrink, r) { + SkString s("hello world!"); + REPORTER_ASSERT(r, s.equals("hello world!")); + s.resize(5); + REPORTER_ASSERT(r, s.equals("hello")); +} + +DEF_TEST(String_resize_grow, r) { + SkString s("hello world!"); + REPORTER_ASSERT(r, s.equals("hello world!")); + s.resize(25); + REPORTER_ASSERT(r, 0 == strcmp(s.c_str(), "hello world!")); // no promises about data past \0 + REPORTER_ASSERT(r, s.size() == 25); +} + +DEF_TEST(String_resize_after_assignment, r) { + SkString s("hello world!"); + SkString t; + t = s; + REPORTER_ASSERT(r, s.equals("hello world!")); + s.resize(25); + REPORTER_ASSERT(r, 0 == strcmp(s.c_str(), "hello world!")); + REPORTER_ASSERT(r, s.size() == 25); + s.resize(5); + REPORTER_ASSERT(r, s.equals("hello")); +} + +static void resize_helper_function(skiatest::Reporter* r, SkString s) { + REPORTER_ASSERT(r, s.equals("hello world!")); + s.resize(5); + REPORTER_ASSERT(r, s.equals("hello")); + s.resize(25); + REPORTER_ASSERT(r, 0 == strcmp(s.c_str(), "hello")); + REPORTER_ASSERT(r, s.size() == 25); +} + +DEF_TEST(String_resize_after_copy_construction, r) { + SkString s("hello world!"); + resize_helper_function(r, s); +}