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.

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

feat: decide whether two C strings are equal and support CHECK_STREQ* and CHECK_STRNE* #1300

Merged
merged 9 commits into from
Dec 26, 2022

Conversation

empiredan
Copy link
Contributor

@empiredan empiredan commented Dec 19, 2022

This PR is to resolve #1299.

There are several types of comparisons for equality between two strings:

  • if both strings are of std::string, to compare case-sensitive equality, we can use operator==
  • if both strings are of std::string, to compare case-insensitive equality, we can use boost::iequals
  • if one string is of std::string and another is of C string, to compare case-sensitive equality we can also use operator==
  • if one string is of std::string and another is of C string, to compare case-insensitive equality we can use strcasecmp
  • if both strings are of C string, to compare case-sensitive equality we can use strcmp
  • if both strings are of C string, to compare case-insensitive equality we can also use strcasecmp
  • to compare the equality of first n bytes, we can use strncmp, strncasecmp, memcmp

However, the problem of strcmp, strcmp, strcasecmp strncmp, strncasecmp and memcmp is that once the compared strings is NULL pointer, these functions may lead to undefined behavior. Thus in this PR we will encapsulate the following functions to solve this problem:

// Decide whether two C strings are equal, even if one of them is NULL.
// The second function is similar except it compares the only first (at most) n bytes
// of both strings.
bool equals(const char *lhs, const char *rhs);
bool equals(const char *lhs, const char *rhs, size_t n);

// Decide whether two C strings are equal, ignoring the case of the characters,
// even if one of them is NULL.
// The second function is similar except it compares the only first (at most) n bytes
// of both strings.
bool iequals(const char *lhs, const char *rhs);
bool iequals(const char *lhs, const char *rhs, size_t n);

// Decide whether two strings one of which is C string are equal, ignoring the
// case of the characters, even if the C string is NULL.
bool iequals(const std::string &lhs, const char *rhs);
bool iequals(const std::string &lhs, const char *rhs, size_t n);
bool iequals(const char *lhs, const std::string &rhs);
bool iequals(const char *lhs, const std::string &rhs, size_t n);

// Decide whether the first n bytes of two memory areas are equal, even if one of them is NULL.
bool mequals(const void *lhs, const void *rhs, size_t n);

This PR will also implement the following macros to help check the equality between C strings and true or false:

  • CHECK_STREQ*
  • CHECK_STRNE*
  • CHECK_TRUE
  • CHECK_FALSE

@github-actions github-actions bot added the cpp label Dec 19, 2022
@acelyc111
Copy link
Member

How about to replace memcmp too?

@empiredan
Copy link
Contributor Author

empiredan commented Dec 20, 2022

How about to replace memcmp too?

OK, I think I can replace with a new function mequals that also processes NULL pointers.

@@ -32,21 +33,21 @@ bool generate_hotkey_request(dsn::replication::detect_hotkey_request &req,
int partition_index,
std::string &err_info)
{
if (!strcasecmp(hotkey_type.c_str(), "read")) {
if (dsn::utils::iequals(hotkey_type.c_str(), "read")) {
Copy link
Member

Choose a reason for hiding this comment

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

how about define the parameter as string_view? so you it's not needed to convet to C string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually string_view is used not only for the string of the printable characters, but also binary bytes. Once string_view is used as the type of the parameters, we have to reimplement comparison ignoring cases based on string_view where binary bytes such as '\0' should be skipped. Therefore here we just declare other functions one parameter of which is declared as std::string. Comparing two std::string-typed strings we can use boost::iequals.

};

for (const auto &test : tests) {
EXPECT_EQ(test.is_equal, dsn::utils::equals(test.lhs, test.rhs));
Copy link
Member

Choose a reason for hiding this comment

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

This style of unit test is wildly used, but it has a shortcoming, when some case failed, it's hard to find out which one failed, right?

Copy link
Contributor Author

@empiredan empiredan Dec 22, 2022

Choose a reason for hiding this comment

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

Yes, once a case has failed it is really hard to look out it. I'll use TEST_P to refactor this.

src/utils/fmt_logging.h Outdated Show resolved Hide resolved
@empiredan empiredan merged commit 86f7e02 into apache:master Dec 26, 2022
@empiredan empiredan mentioned this pull request Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: decide whether two C strings are equal and support CHECK_STREQ* and CHECK_STRNE*
3 participants