Skip to content

sstring: add == comparison operators - #1654

Merged
avikivity merged 1 commit into
scylladb:masterfrom
tchaikov:sstring-equal
May 22, 2023
Merged

sstring: add == comparison operators#1654
avikivity merged 1 commit into
scylladb:masterfrom
tchaikov:sstring-equal

Conversation

@tchaikov

@tchaikov tchaikov commented May 16, 2023

Copy link
Copy Markdown
Contributor

before this change, we only implemented the <=> operator with
auto as its template parameter as a member of sstring class.
and the == and != operator with basic_sstring as members of
sstring class. but this is not enough for some of our use cases,
where we also want to compare sstring with, for instance,
std::string and plain C strings, just like how we compare
an instance of std::string with std::string and plain C strings.
these are legitimate use cases, as we expect sstring as
a drop-in replacement of std::string under most circumstances.

because, even in C++20, the existing operator==(const basic_sstring&)
fails to be selected as a rewrite candidate when compiler tries to
compile a comparison expression like: "a" == sstring("a"), where
the LHS is a plain C string, because the operator!= prevents it to
do so. without the operator!=, the compiler could have synthesized
a candidate of operator==. see https://eel.is/c++draft/over.match.oper#4

the related paragraph of the draft is quoted below:

A non-template function or function template F named operator==
is a rewrite target with first operand o unless a search for the
name operator!= in the scope S from the instantiation context of
the operator expression finds a function or function template
that would correspond ([basic.scope.scope]) to F if its name were
operator==, where S is the scope of the class type of o if F is a
class member, and the namespace scope of which F is a member
otherwise.

so, one solution is to simply drop the operator!=(const basic_sstring&).
that actually gets the tests added in this change compile. but the
synthesized operator performs the implicit conversion by calling
the constructor of sstring, which performs a deep copy of another
operand in case whose type is not sstring.

as yet another alternative, we could define an (inline) friend of
operator== and operator!=, they would allow implicit conversion
to sstring if one operator is not an sstring. but again, this hurts
the performance because of the deep copy.

so, in this change, to address the needs from the functionality
and performance perspectives, we add the == operators for
std::basic_string<char_type> and const char*. the new overloads
are selected based on their counterparts of std::string, where
std::string has

  • operator==(std::basic_string<char_type...>&) const
  • operator==(const char*) const

so we are adding the same set of == operators for sstring.

this should fulfill the needs of the use case where == and
!= operators are used with C++20. it is worth nothing that,
this change does not address the use cases in C++17, where
the LHS of the == comparison expression is not sstring. but
since this change should also benefit the applications in
C++17, the new operators are not guarded by any C++ language
or C++ library feature testing macro guards.

please note, we continue using template to implement the operators,
in order to match with the behavior of std::string. this disallows the
implicit conversions from other types to the operands of the comparsion
operator.

a test is added accordingly (courtesy of Avi).

@tchaikov tchaikov changed the title sstring: add == comparison operators sstring: add == comparison operator May 16, 2023
before this change, we only implemented the <=> operator with
`auto` as its template parameter as a member of sstring class.
and the == and != operator with basic_sstring as members of
sstring class. but this is not enough for some of our use cases,
where we also want to compare sstring with, for instance,
std::string and plain C strings, just like how we compare
an instance of std::string with std::string and plain C strings.
these are legitimate use cases, as we expect sstring as
a drop-in replacement of std::string under most circumstances.

because, even in C++20, the existing operator==(const basic_sstring&)
fails to be selected as a rewrite candidate when compiler tries to
compile a comparison expression like: "a" == sstring("a"), where
the LHS is a plain C string, because the operator!= prevents it to
do so. without the operator!=, the compiler could have synthesized
a candidate of operator==. see https://eel.is/c++draft/over.match.oper#4

the related paragraph of the draft is quoted below:

> A non-template function or function template F named operator==
> is a rewrite target with first operand o unless a search for the
> name operator!= in the scope S from the instantiation context of
> the operator expression finds a function or function template
> that would correspond ([basic.scope.scope]) to F if its name were
> operator==, where S is the scope of the class type of o if F is a
> class member, and the namespace scope of which F is a member
> otherwise.

so, one solution is to simply drop the `operator!=(const basic_sstring&)`.
that actually gets the tests added in this change compile. but the
synthesized operator performs the implicit conversion by calling
the constructor of sstring, which performs a deep copy of another
operand in case whose type is not sstring.

as yet another alternative, we could define an (inline) friend of
operator== and operator!=, they would allow implicit conversion
to sstring if one operator is not an sstring. but again, this hurts
the performance because of the deep copy.

so, in this change, to address the needs from the functionality
and performance perspectives, we add the == operators for
std::basic_string<char_type> and const char*. the new overloads
are selected based on their counterparts of std::string, where
std::string has

* operator==(std::basic_string<char_type...>&) const
* operator==(const char*) const

so we are adding the same set of == operators for sstring.

this should fulfill the needs of the use case where == and
!= operators are used with C++20. it is worth nothing that,
this change does not address the use cases in C++17, where
the LHS of the == comparison expression is not sstring. but
since this change should also benefit the applications in
C++17, the new operators are not guarded by any C++ language
or C++ library feature testing macro guards.

please note, we continue using template to implement the operators,
in order to match with the behavior of std::string. this disallows the
implicit conversions from other types to the operands of the comparsion
operator.

a test is added accordingly (courtesy of Avi).

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>
@tchaikov

Copy link
Copy Markdown
Contributor Author

this change enables clang++16 + libstdc++ from gcc-13 to compile test_unit_sstring without the help of scylladb/scylladb#13893 .

@tchaikov tchaikov changed the title sstring: add == comparison operator sstring: add == comparison operators May 17, 2023

@nyh nyh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me, but I doubt I understood all the language-lawyery details so @avikivity please review (you looked into this same problem too).

@tchaikov

Copy link
Copy Markdown
Contributor Author

@avikivity hi Avi, could you please take a look?

@avikivity

Copy link
Copy Markdown
Member

Very good indeed (only read the changelog so far).

@avikivity
avikivity merged commit d5bd2ed into scylladb:master May 22, 2023
@tchaikov
tchaikov deleted the sstring-equal branch May 22, 2023 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants