-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sstring: implement push_back() #2106
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -346,6 +346,29 @@ public: | |
return *this; | ||
} | ||
|
||
/** | ||
* Append the given character to the end of the string | ||
* @param ch The character to append | ||
* @note unlike @c std::basic_string, sstring does not preallocate, so | ||
* this call always lead to reallocation if external storage is | ||
* used. | ||
*/ | ||
constexpr void push_back(char_type ch) { | ||
if (is_internal()) { | ||
if (static_cast<Size>(u.internal.size) < max_size) { | ||
u.internal.str[u.internal.size++] = ch; | ||
if (NulTerminate) { | ||
u.internal.str[u.internal.size] = '\0'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if at this point u.internal.size already reached max_size? |
||
} | ||
return; | ||
} | ||
} | ||
basic_sstring new_str(initialized_later(), u.external.size + 1); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if we reached here despite is_internal() (because we reached max size)? Isn't u.external.size wrong? |
||
std::copy(begin(), end(), new_str.begin()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The documentation https://en.cppreference.com/w/cpp/string/basic_string/push_back says that push_back() should have "amortized constant" complexity. It means you cannot do this copy on every push_back() call - it would make it O(n) complexity. I think you need to multiply the size of the string by something (1.1 or 2 or whatever) and until you reach the preallocated size, just copy one character and not the entire string. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't have unused capacity, so can't do that. I think push_back is bad since it can't be implemented efficiently and will lead to accidentally quadratic users. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we can't do that, we shouldn't implement this function. Its complexity is part of its API. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how about trading the implementation for a comment, noting that we don't intend to implement this because of the performance issues due to the inherent design decisions made by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nobody will read a comment on push_back. Everyone already knows how it works. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @avikivity hi Avi, i didn't mean to add a comment near by this function. i wanted to put a comment in the place of it explaining why we don't have it. to explain that it is a design decision. |
||
new_str.u.external.str[new_str.u.external.size - 1] = ch; | ||
*this = std::move(new_str); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not familiar with NulTerminate but don't we need to do it here as well? |
||
} | ||
|
||
/** | ||
* Resize string and use the specified @c op to modify the content and the length | ||
* @param n new size | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -199,6 +199,27 @@ BOOST_AUTO_TEST_CASE(test_append) { | |
BOOST_REQUIRE_EQUAL(sstring("aba").append("1234", 0), "aba"); | ||
} | ||
|
||
BOOST_AUTO_TEST_CASE(test_push_back) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: push_back() against empty string looks like good corner case test |
||
{ | ||
// append in internal storage | ||
basic_sstring<char, uint32_t, 15> s("0123456789"); | ||
s.push_back('a'); | ||
BOOST_REQUIRE_EQUAL(s, "0123456789a"); | ||
} | ||
{ | ||
// append causing spilling to external storage | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would do this test in a loop, for many sizes, not just 15 - e.g. from size 0 to 50. I don't know if 15 exactly is the cutoff point, or maybe 14 or 16? I'm worried that the above code has multiple bugs (I pointed out my suspicions above) and this test doesn't make me confident. |
||
basic_sstring<char, uint32_t, 15> s("0123456789abcde"); | ||
s.push_back('f'); | ||
BOOST_REQUIRE_EQUAL(s, "0123456789abcdef"); | ||
} | ||
{ | ||
// append with external storage | ||
basic_sstring<char, uint32_t, 15> s("0123456789abcdef"); | ||
s.push_back('g'); | ||
BOOST_REQUIRE_EQUAL(s, "0123456789abcdefg"); | ||
} | ||
} | ||
|
||
BOOST_AUTO_TEST_CASE(test_replace) { | ||
BOOST_REQUIRE_EQUAL(sstring("abc").replace(1,1, "xyz", 1), "axc"); | ||
BOOST_REQUIRE_EQUAL(sstring("abc").replace(3,2, "xyz", 2), "abcxy"); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should add padding() here, see the constructor from initialized_later.