acelyc111 commented on code in PR #1300:
URL:
https://github.com/apache/incubator-pegasus/pull/1300#discussion_r1053936365
##########
src/shell/commands/detect_hotkey.cpp:
##########
@@ -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")) {
Review Comment:
how about define the parameter as `string_view`? so you it's not needed to
convet to C string.
##########
src/utils/fmt_logging.h:
##########
@@ -107,6 +162,11 @@
var1 < var2, _v1 < _v2, "{} vs {} {}", _v1, _v2,
fmt::format(__VA_ARGS__)); \
} while (false)
+#define CHECK_STREQ(var1, var2) CHECK_STREQ_MSG(var1, var2, "")
+#define CHECK_STRNE(var1, var2) CHECK_STRNE_MSG(var1, var2, "")
+#define CHECK_STRCASEEQ(var1, var2) CHECK_STRCASEEQ_MSG(var1, var2, "")
Review Comment:
According to the macro names, it's confused to judge which one is case
sensitive.
I found there is no caller currently, can you remove `CHECK_STRCASEEQ/NE`?
##########
src/utils/test/utils.cpp:
##########
@@ -115,6 +115,52 @@ TEST(core, check_c_string_empty)
}
}
+TEST(core, check_c_string_equal)
+{
+ struct test_case
+ {
+ const char *lhs;
+ const char *rhs;
+ bool is_equal;
+ } tests[] = {
+ {nullptr, nullptr, true}, {nullptr, "", false}, {nullptr, "a", false},
+ {nullptr, "abc", false}, {"", nullptr, false}, {"a", nullptr, false},
+ {"abc", nullptr, false}, {"", "", true}, {"", "a", false},
+ {"", "abc", false}, {"a", "", false}, {"abc", "", false},
+ {"a", "a", true}, {"a", "A", false}, {"A", "A", true},
+ {"abc", "abc", true}, {"aBc", "abc", false}, {"abc", "ABC", false},
+ {"a", "abc", false}, {"A", "abc", false}, {"abc", "a", false},
+ {"Abc", "a", false},
+ };
+
+ for (const auto &test : tests) {
+ EXPECT_EQ(test.is_equal, dsn::utils::equals(test.lhs, test.rhs));
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
