You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by "WHBANG (via GitHub)" <gi...@apache.org> on 2023/02/02 05:38:11 UTC
[GitHub] [incubator-pegasus] WHBANG opened a new pull request, #1338: feat(utils): add a tool method find_string_ prefix in utils
WHBANG opened a new pull request, #1338:
URL: https://github.com/apache/incubator-pegasus/pull/1338
### What problem does this PR solve? <!--add issue link with summary if exists-->
https://github.com/apache/incubator-pegasus/issues/1337
- Unit test
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] empiredan commented on a diff in pull request #1338: feat(utils): add an utility function to find string prefix by separator
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan commented on code in PR #1338:
URL: https://github.com/apache/incubator-pegasus/pull/1338#discussion_r1095319925
##########
src/utils/strings.cpp:
##########
@@ -417,5 +417,14 @@ std::string string_md5(const char *buffer, unsigned length)
return result;
}
+
+std::string find_string_prefix(const std::string &input, char separator)
+{
+ std::size_t current = input.find(separator);
+ if (current == 0 || current == std::string::npos) {
+ return "";
Review Comment:
Initializing with c-style empty string literal may be less efficient for compilers with older version even if optimization is enabled. Use default constructor instead:
```suggestion
return std::string();
```
##########
src/utils/strings.cpp:
##########
@@ -417,5 +417,14 @@ std::string string_md5(const char *buffer, unsigned length)
return result;
}
+
+std::string find_string_prefix(const std::string &input, char separator)
+{
+ std::size_t current = input.find(separator);
Review Comment:
```suggestion
auto current = input.find(separator);
```
##########
src/utils/test/utils.cpp:
##########
@@ -443,6 +443,26 @@ TEST(core, dlink)
EXPECT_TRUE(count == 0);
}
+TEST(core, find_string_prefix)
+{
+ struct test_case
+ {
+ std::string input;
+ char separator;
+ std::string expected_prefix;
+ } tests[] = {{"", ' ', ""},
+ {"abc.def", ' ', ""},
+ {"abc.def", '.', "abc"},
+ {"ab.cd.ef", '.', "ab"},
+ {"abc...def", '.', "abc"},
+ {".abc.def", '.', ""}};
+ std::string actual_output;
+ for (const auto &test : tests) {
+ actual_output = find_string_prefix(test.input, test.separator);
Review Comment:
```suggestion
auto actual_output = find_string_prefix(test.input, test.separator);
```
##########
src/utils/test/utils.cpp:
##########
@@ -443,6 +443,26 @@ TEST(core, dlink)
EXPECT_TRUE(count == 0);
}
+TEST(core, find_string_prefix)
+{
+ struct test_case
+ {
+ std::string input;
+ char separator;
+ std::string expected_prefix;
+ } tests[] = {{"", ' ', ""},
Review Comment:
Could add cases that the separator is just the whole source string, and the source string only include separators, such as:
```c++
{{" ", ' ', ""}},
{{"..", '.', ""}},
```
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 merged pull request #1338: feat(utils): add an utility function to find string prefix by separator
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 merged PR #1338:
URL: https://github.com/apache/incubator-pegasus/pull/1338
--
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: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org