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: add more accessors #1755

Merged
merged 5 commits into from
Jul 26, 2023
Merged

sstring: add more accessors #1755

merged 5 commits into from
Jul 26, 2023

Conversation

tchaikov
Copy link
Contributor

in this series, more accessors are added to seastar::sstring. the goal is to make sstring more standard compliant, so that it can be used in lieu of std::string when appropriate.

@tchaikov
Copy link
Contributor Author

this change was inspired by https://github.com/scylladb/scylladb/pull/14800/files#diff-7b428ff10eee5bec5a6f74223c8dee8d61085bbaf290538bd3ae6e8c4c606efdR1139 where the author uses

 if (!fn_name.find("castas")) {
     // something we should do when fn_name starts with "castas" 
 }

IMHO, this is not quite readable.

@tchaikov
sstring: add sstring::find() overloads
416a247 
to be compliant with the C++20 standard, which requires that
a string should have following find() overloads:

template<class T>
  constexpr size_type find(const T& t, size_type pos = 0) const noexcept
// where T should be convertible to basic_string_view
constexpr size_type find(const charT* s, size_type pos, size_type n) const;
constexpr size_type find(const charT* s, size_type pos = 0) const;

so let's provide these overloads, so that sstring can be used in lieu of
std::string, when appropriate.

see also https://eel.is/c++draft/basic.string#string.ops

Signed-off-by: Kefu Chai <[email protected]>
@tchaikov
sstring: add sstring::front()
e055ef2 
as required by the C++20 standard, std::string should provide

constexpr const charT& front() const;
constexpr charT& front();

so let's add them to sstring. so that sstring can be used in lieu of
std::string, when appropriate.

see also https://eel.is/c++draft/basic.string#string.access

Signed-off-by: Kefu Chai <[email protected]>
@tchaikov
sstring: add sstring::starts_with()
368c7e1 
as required by the C++20 standard, std::string should provide

constexpr bool starts_with(basic_string_view<charT, traits> x) const noexcept;
constexpr bool starts_with(charT x) const noexcept;
constexpr bool starts_with(const charT* x) const;

so let's add them to sstring. . so that sstring can be used in lieu of
std::string, when appropriate.

see also https://eel.is/c++draft/string.starts.with

Signed-off-by: Kefu Chai <[email protected]>
@tchaikov
sstring: add sstring::ends_with()
aebef64 
as required by the C++20 standard, std::string should provide

constexpr bool ends_with(basic_string_view<charT, traits> x) const noexcept;
constexpr bool ends_with(charT x) const noexcept;
constexpr bool ends_with(const charT* x) const;

so let's add them to sstring. . so that sstring can be used in lieu of
std::string, when appropriate.

see also https://eel.is/c++draft/string.ends.with

Signed-off-by: Kefu Chai <[email protected]>
@tchaikov
sstring: add sstring::contains()
3ea436e 
as required by the C++20 standard, std::string should provide

constexpr bool contains(basic_string_view<charT, traits> x) const noexcept;
constexpr bool contains(charT x) const noexcept;
constexpr bool contains(const charT* x) const;

so let's add them to sstring. . so that sstring can be used in lieu of
std::string, when appropriate.

see also https://eel.is/c++draft/string.contains

Signed-off-by: Kefu Chai <[email protected]>
xemul
xemul reviewed Jul 24, 2023
View reviewed changes
include/seastar/core/sstring.hh
}

template<class StringViewLike,
std::enable_if_t<std::is_convertible_v<StringViewLike,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sanity check: usage of SEASTAR_CONCEPT is now not mandatory, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, disregard, it's not a concept 🤦‍♂️

Copy link
Contributor Author

@tchaikov tchaikov Jul 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think these helpers are also handy to C++17 users, so chose to avoid using C++20.

@xemul xemul closed this in ba1e39d Jul 26, 2023
@xemul xemul merged commit ba1e39d into scylladb:master Jul 26, 2023
12 checks passed
@nyh
Copy link
Contributor

nyh commented Jul 26, 2023

in this series, more accessors are added to seastar::sstring. the goal is to make sstring more standard compliant, so that it can be used in lieu of std::string when appropriate.

I agree that seastar::sstring should be more compatible with std::string, but I don't agree with your wish to use seastar::sstring in lieu of std::string - I think it should be the other way around, Seastar applications should avoid seastar::sstring as much as possible - see #634

@tchaikov tchaikov deleted the sstring-accessors branch July 27, 2023 03:01
@tchaikov
Copy link
Contributor Author

sorry for the misunderstanding. i think, i, well, again, sent the wrong message to the reviewer and greater community. i am not a big fan of sstring or homebrew string solutions. on the contrary, i want to reduce the pain of switching over to std::string or std::string_view. the goal of adding more accessors to sstring is not to make it "better", but to encourage developers to replace it with the string-alike class provided by the standard library.

but @xemul mentioned at

seastar/include/seastar/core/sstring.hh

Lines 56 to 59 in ba1e39d

// Older std::string used atomic reference counting and had no small-buffer-optimization.
// At some point the new std::string ABI improved -- no reference counting plus the small
// buffer optimization. However, aliasing seastar::sstring to std::string still ends up
// with a small performance degradation. (FIXME?)
that there was a "a small performance degradation". i need to verify this.

BTW, the initiative to remove the formatters for standard library containers in Seastar and ScyllaDB is of the same spirit, despite it is difficult in the sense of the amount of work.

@avikivity
Copy link
Member

I think that sstring should mirror std::string as far as possible. It would have been better to use std::string, but we measured a significant performance degradation with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xemul xemul xemul left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tchaikov @nyh @avikivity @xemul