Skip to content
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.

Sign up for GitHub

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

Jump to bottom

sstring: implement push_back() #2106

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

sstring: implement push_back() #2106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions include/seastar/core/sstring.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

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.

u.internal.str[u.internal.size++] = ch;
if (NulTerminate) {
u.internal.str[u.internal.size] = '\0';
Copy link
Contributor

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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?
Please make sure you tests check the case of an empty string growing and growing until passing the internal size and see it still works.

std::copy(begin(), end(), new_str.begin());
Copy link
Contributor

@nyh nyh Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@tchaikov tchaikov Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The 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 sstring (not because that we are lazy =).

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand Down
21 changes: 21 additions & 0 deletions tests/unit/sstring_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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.
Also please check the NulTerminate option, whatever it is.

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");
Expand Down