You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/06/21 08:41:00 UTC
[GitHub] [incubator-kvrocks] PragmaTwice opened a new pull request, #644: Refactor Trim/Split/Split2KV functions in util.h
PragmaTwice opened a new pull request, #644:
URL: https://github.com/apache/incubator-kvrocks/pull/644
- Solve #581
- Refactor Split and Split2KV
- Optimize some signature to const reference.
- Add some unit test for string utils
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #644: Refactor Trim/Split/Split2KV functions in util.h
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #644:
URL: https://github.com/apache/incubator-kvrocks/pull/644#discussion_r902440112
##########
src/util.cc:
##########
@@ -371,7 +371,14 @@ std::string Trim(std::string in, const std::string &chars) {
std::vector<std::string> Split(const std::string &in, const std::string &delim) {
std::vector<std::string> out;
- if (in.empty() || delim.empty()) return out;
+ if (in.empty()) {
+ return out;
+ }
+
+ if (delim.empty()) {
Review Comment:
Thanks, I'll fix it.
BTW, this function has some difference than some well known `split`: it split strings regarding to regex "a|b|c|..." where delim="abc...", so I am considering to add another split that split strings just regarding to the provided delimiter.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #644: Refactor Trim/Split/Split2KV functions in util.h
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #644:
URL: https://github.com/apache/incubator-kvrocks/pull/644#discussion_r902833499
##########
src/util.cc:
##########
@@ -359,42 +359,54 @@ std::string ToLower(std::string in) {
return in;
}
-void Trim(const std::string &in, const std::string &chars, std::string *out) {
- out->clear();
- if (in.empty()) return;
- out->assign(in);
- out->erase(0, out->find_first_not_of(chars));
- out->erase(out->find_last_not_of(chars)+1);
+std::string Trim(std::string in, const std::string &chars) {
+ if (in.empty()) return in;
+
+ in.erase(0, in.find_first_not_of(chars));
+ in.erase(in.find_last_not_of(chars) + 1);
+
+ return in;
}
-void Split(std::string in, std::string delim, std::vector<std::string> *out) {
- if (in.empty() || !out) return;
- out->clear();
+std::vector<std::string> Split(const std::string &in, const std::string &delim) {
+ std::vector<std::string> out;
+
+ if (in.empty()) {
+ return out;
+ }
+
+ if (delim.empty()) {
+ out.resize(in.size());
+ std::transform(in.begin(), in.end(), out.begin(),
+ [](char c) -> std::string { return {c}; });
+ return out;
+ }
- std::string::size_type pos = 0;
- std::string elem, trimed_elem;
+ size_t begin = 0, end = in.find_first_of(delim);
do {
- pos = in.find_first_of(delim);
- elem = in.substr(0, pos);
- Trim(elem, delim, &trimed_elem);
- if (!trimed_elem.empty()) out->push_back(trimed_elem);
- in = in.substr(pos+1);
- } while (pos != std::string::npos);
+ std::string elem = in.substr(begin, end - begin);
+ if (!elem.empty()) out.push_back(std::move(elem));
Review Comment:
I think `Split("1 2", " ")` should return `{"1", "2"}`, and it is same as previous behavior.
`Split("1 2","")` returns `{"1", " ", "2"}`.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #644: Refactor Trim/Split/Split2KV functions in util.h
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #644:
URL: https://github.com/apache/incubator-kvrocks/pull/644#discussion_r902448104
##########
src/util.cc:
##########
@@ -371,7 +371,14 @@ std::string Trim(std::string in, const std::string &chars) {
std::vector<std::string> Split(const std::string &in, const std::string &delim) {
std::vector<std::string> out;
- if (in.empty() || delim.empty()) return out;
+ if (in.empty()) {
+ return out;
+ }
+
+ if (delim.empty()) {
Review Comment:
Yes. We don't have to follow other system's design, but we should make our own utils semantic clear.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #644: Refactor Trim/Split/Split2KV functions in util.h
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #644:
URL: https://github.com/apache/incubator-kvrocks/pull/644#discussion_r903255268
##########
src/util.cc:
##########
@@ -538,25 +550,29 @@ std::string StringToHex(const std::string &input) {
return output;
}
+constexpr std::uint64_t ExpTo1024(size_t n) {
+ return static_cast<std::uint64_t>(1) << (10 * n);
+}
+
void BytesToHuman(char *buf, size_t size, uint64_t n) {
double d;
- if (n < 1024) {
+ if (n < ExpTo1024(1)) {
Review Comment:
It just replace `1024*1024*...*1024` to something like `1024^n`, so I don't think it will affect readability.
> And this change is not related to theme
Agree. Maybe move it to a separate PR.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #644: Refactor Trim/Split/Split2KV functions in util.h
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #644:
URL: https://github.com/apache/incubator-kvrocks/pull/644#discussion_r902547716
##########
src/util.cc:
##########
@@ -371,7 +371,14 @@ std::string Trim(std::string in, const std::string &chars) {
std::vector<std::string> Split(const std::string &in, const std::string &delim) {
std::vector<std::string> out;
- if (in.empty() || delim.empty()) return out;
+ if (in.empty()) {
+ return out;
+ }
+
+ if (delim.empty()) {
Review Comment:
done
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #644: Refactor Trim/Split/Split2KV functions in util.h
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #644:
URL: https://github.com/apache/incubator-kvrocks/pull/644#discussion_r902833499
##########
src/util.cc:
##########
@@ -359,42 +359,54 @@ std::string ToLower(std::string in) {
return in;
}
-void Trim(const std::string &in, const std::string &chars, std::string *out) {
- out->clear();
- if (in.empty()) return;
- out->assign(in);
- out->erase(0, out->find_first_not_of(chars));
- out->erase(out->find_last_not_of(chars)+1);
+std::string Trim(std::string in, const std::string &chars) {
+ if (in.empty()) return in;
+
+ in.erase(0, in.find_first_not_of(chars));
+ in.erase(in.find_last_not_of(chars) + 1);
+
+ return in;
}
-void Split(std::string in, std::string delim, std::vector<std::string> *out) {
- if (in.empty() || !out) return;
- out->clear();
+std::vector<std::string> Split(const std::string &in, const std::string &delim) {
+ std::vector<std::string> out;
+
+ if (in.empty()) {
+ return out;
+ }
+
+ if (delim.empty()) {
+ out.resize(in.size());
+ std::transform(in.begin(), in.end(), out.begin(),
+ [](char c) -> std::string { return {c}; });
+ return out;
+ }
- std::string::size_type pos = 0;
- std::string elem, trimed_elem;
+ size_t begin = 0, end = in.find_first_of(delim);
do {
- pos = in.find_first_of(delim);
- elem = in.substr(0, pos);
- Trim(elem, delim, &trimed_elem);
- if (!trimed_elem.empty()) out->push_back(trimed_elem);
- in = in.substr(pos+1);
- } while (pos != std::string::npos);
+ std::string elem = in.substr(begin, end - begin);
+ if (!elem.empty()) out.push_back(std::move(elem));
Review Comment:
I think `Split("1 2", " ")` should return `{"1", "2"}`, and it is same as previous behavior.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #644: Refactor Trim/Split/Split2KV functions in util.h
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #644:
URL: https://github.com/apache/incubator-kvrocks/pull/644#discussion_r902833499
##########
src/util.cc:
##########
@@ -359,42 +359,54 @@ std::string ToLower(std::string in) {
return in;
}
-void Trim(const std::string &in, const std::string &chars, std::string *out) {
- out->clear();
- if (in.empty()) return;
- out->assign(in);
- out->erase(0, out->find_first_not_of(chars));
- out->erase(out->find_last_not_of(chars)+1);
+std::string Trim(std::string in, const std::string &chars) {
+ if (in.empty()) return in;
+
+ in.erase(0, in.find_first_not_of(chars));
+ in.erase(in.find_last_not_of(chars) + 1);
+
+ return in;
}
-void Split(std::string in, std::string delim, std::vector<std::string> *out) {
- if (in.empty() || !out) return;
- out->clear();
+std::vector<std::string> Split(const std::string &in, const std::string &delim) {
+ std::vector<std::string> out;
+
+ if (in.empty()) {
+ return out;
+ }
+
+ if (delim.empty()) {
+ out.resize(in.size());
+ std::transform(in.begin(), in.end(), out.begin(),
+ [](char c) -> std::string { return {c}; });
+ return out;
+ }
- std::string::size_type pos = 0;
- std::string elem, trimed_elem;
+ size_t begin = 0, end = in.find_first_of(delim);
do {
- pos = in.find_first_of(delim);
- elem = in.substr(0, pos);
- Trim(elem, delim, &trimed_elem);
- if (!trimed_elem.empty()) out->push_back(trimed_elem);
- in = in.substr(pos+1);
- } while (pos != std::string::npos);
+ std::string elem = in.substr(begin, end - begin);
+ if (!elem.empty()) out.push_back(std::move(elem));
Review Comment:
I think it seems more reasonable if `Split("1 2", " ")` returns `{"1", "2"}`, and it is same as previous behavior.
`Split("1 2","")` returns `{"1", " ", "2"}`.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #644: Refactor Trim/Split/Split2KV functions in util.h
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #644:
URL: https://github.com/apache/incubator-kvrocks/pull/644#discussion_r903366097
##########
src/util.cc:
##########
@@ -538,25 +550,29 @@ std::string StringToHex(const std::string &input) {
return output;
}
+constexpr std::uint64_t ExpTo1024(size_t n) {
+ return static_cast<std::uint64_t>(1) << (10 * n);
+}
+
void BytesToHuman(char *buf, size_t size, uint64_t n) {
double d;
- if (n < 1024) {
+ if (n < ExpTo1024(1)) {
Review Comment:
These changes are reverted, I'll prepare a seperate PR to illustrate them.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #644: Refactor Trim/Split/Split2KV functions in util.h
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #644:
URL: https://github.com/apache/incubator-kvrocks/pull/644#discussion_r902397536
##########
src/util.cc:
##########
@@ -371,7 +371,14 @@ std::string Trim(std::string in, const std::string &chars) {
std::vector<std::string> Split(const std::string &in, const std::string &delim) {
std::vector<std::string> out;
- if (in.empty() || delim.empty()) return out;
+ if (in.empty()) {
+ return out;
+ }
+
+ if (delim.empty()) {
Review Comment:
Sorry. After take a look at Java's `Spring.split` and C++20's [`split_view`](https://en.cppreference.com/w/cpp/ranges/split_view), its seems empty delim will cause output a string vector contains every character as a string member.
```java
jshell> "123".split("")
$1 ==> String[3] { "1", "2", "3" }
jshell> "1 2 3".split("")
$2 ==> String[5] { "1", " ", "2", " ", "3" }
```
```cpp
#include <iostream>
#include <iomanip>
#include <ranges>
#include <string_view>
int main() {
constexpr std::string_view words{"Hello-_-C++-_-20-_-!"};
constexpr std::string_view delim{""};
for (const auto word : std::views::split(words, delim)) {
std::cout << std::quoted(std::string_view(word.begin(), word.end())) << ' ';
}
}
// "H" "e" "l" "l" "o" "-" "_" "-" "C" "+" "+" "-" "_" "-" "2" "0" "-" "_" "-" "!"
// https://wandbox.org/permlink/5BZQHqUYd36t0WbQ
```
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #644: Refactor Trim/Split/Split2KV functions in util.h
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #644:
URL: https://github.com/apache/incubator-kvrocks/pull/644#issuecomment-1162997392
Thanks everyone! Merging...
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] ShooterIT commented on a diff in pull request #644: Refactor Trim/Split/Split2KV functions in util.h
Posted by GitBox <gi...@apache.org>.
ShooterIT commented on code in PR #644:
URL: https://github.com/apache/incubator-kvrocks/pull/644#discussion_r903253106
##########
src/util.cc:
##########
@@ -538,25 +550,29 @@ std::string StringToHex(const std::string &input) {
return output;
}
+constexpr std::uint64_t ExpTo1024(size_t n) {
+ return static_cast<std::uint64_t>(1) << (10 * n);
+}
+
void BytesToHuman(char *buf, size_t size, uint64_t n) {
double d;
- if (n < 1024) {
+ if (n < ExpTo1024(1)) {
Review Comment:
actually this code is copied from redis.
but for this change, i don't feel it is clear, maybe we can define `KB MB GB TB PB`.
And this change is not related to theme.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #644: Refactor Trim/Split/Split2KV functions in util.h
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #644:
URL: https://github.com/apache/incubator-kvrocks/pull/644#discussion_r903205197
##########
src/util.cc:
##########
@@ -359,42 +359,54 @@ std::string ToLower(std::string in) {
return in;
}
-void Trim(const std::string &in, const std::string &chars, std::string *out) {
- out->clear();
- if (in.empty()) return;
- out->assign(in);
- out->erase(0, out->find_first_not_of(chars));
- out->erase(out->find_last_not_of(chars)+1);
+std::string Trim(std::string in, const std::string &chars) {
+ if (in.empty()) return in;
+
+ in.erase(0, in.find_first_not_of(chars));
+ in.erase(in.find_last_not_of(chars) + 1);
+
+ return in;
}
-void Split(std::string in, std::string delim, std::vector<std::string> *out) {
- if (in.empty() || !out) return;
- out->clear();
+std::vector<std::string> Split(const std::string &in, const std::string &delim) {
+ std::vector<std::string> out;
+
+ if (in.empty()) {
+ return out;
+ }
+
+ if (delim.empty()) {
+ out.resize(in.size());
+ std::transform(in.begin(), in.end(), out.begin(),
+ [](char c) -> std::string { return {c}; });
+ return out;
+ }
- std::string::size_type pos = 0;
- std::string elem, trimed_elem;
+ size_t begin = 0, end = in.find_first_of(delim);
do {
- pos = in.find_first_of(delim);
- elem = in.substr(0, pos);
- Trim(elem, delim, &trimed_elem);
- if (!trimed_elem.empty()) out->push_back(trimed_elem);
- in = in.substr(pos+1);
- } while (pos != std::string::npos);
+ std::string elem = in.substr(begin, end - begin);
+ if (!elem.empty()) out.push_back(std::move(elem));
Review Comment:
Yes, this `split` function has a bit different from most languages
since we want to ignore the empty value.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #644: Refactor Trim/Split/Split2KV functions in util.h
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #644:
URL: https://github.com/apache/incubator-kvrocks/pull/644#discussion_r902807971
##########
src/util.cc:
##########
@@ -359,42 +359,54 @@ std::string ToLower(std::string in) {
return in;
}
-void Trim(const std::string &in, const std::string &chars, std::string *out) {
- out->clear();
- if (in.empty()) return;
- out->assign(in);
- out->erase(0, out->find_first_not_of(chars));
- out->erase(out->find_last_not_of(chars)+1);
+std::string Trim(std::string in, const std::string &chars) {
+ if (in.empty()) return in;
+
+ in.erase(0, in.find_first_not_of(chars));
+ in.erase(in.find_last_not_of(chars) + 1);
+
+ return in;
}
-void Split(std::string in, std::string delim, std::vector<std::string> *out) {
- if (in.empty() || !out) return;
- out->clear();
+std::vector<std::string> Split(const std::string &in, const std::string &delim) {
+ std::vector<std::string> out;
+
+ if (in.empty()) {
+ return out;
+ }
+
+ if (delim.empty()) {
+ out.resize(in.size());
+ std::transform(in.begin(), in.end(), out.begin(),
+ [](char c) -> std::string { return {c}; });
+ return out;
+ }
- std::string::size_type pos = 0;
- std::string elem, trimed_elem;
+ size_t begin = 0, end = in.find_first_of(delim);
do {
- pos = in.find_first_of(delim);
- elem = in.substr(0, pos);
- Trim(elem, delim, &trimed_elem);
- if (!trimed_elem.empty()) out->push_back(trimed_elem);
- in = in.substr(pos+1);
- } while (pos != std::string::npos);
+ std::string elem = in.substr(begin, end - begin);
+ if (!elem.empty()) out.push_back(std::move(elem));
Review Comment:
`Split("1 2", " ")` should normally return `{"1", "", "2"}`. But with this condition, it returns `{"1", "2"}`.
I don't know if kvrocks depends on this behavior but point out it's diverged from other libraries.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #644: Refactor Trim/Split/Split2KV functions in util.h
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #644:
URL: https://github.com/apache/incubator-kvrocks/pull/644#discussion_r902833499
##########
src/util.cc:
##########
@@ -359,42 +359,54 @@ std::string ToLower(std::string in) {
return in;
}
-void Trim(const std::string &in, const std::string &chars, std::string *out) {
- out->clear();
- if (in.empty()) return;
- out->assign(in);
- out->erase(0, out->find_first_not_of(chars));
- out->erase(out->find_last_not_of(chars)+1);
+std::string Trim(std::string in, const std::string &chars) {
+ if (in.empty()) return in;
+
+ in.erase(0, in.find_first_not_of(chars));
+ in.erase(in.find_last_not_of(chars) + 1);
+
+ return in;
}
-void Split(std::string in, std::string delim, std::vector<std::string> *out) {
- if (in.empty() || !out) return;
- out->clear();
+std::vector<std::string> Split(const std::string &in, const std::string &delim) {
+ std::vector<std::string> out;
+
+ if (in.empty()) {
+ return out;
+ }
+
+ if (delim.empty()) {
+ out.resize(in.size());
+ std::transform(in.begin(), in.end(), out.begin(),
+ [](char c) -> std::string { return {c}; });
+ return out;
+ }
- std::string::size_type pos = 0;
- std::string elem, trimed_elem;
+ size_t begin = 0, end = in.find_first_of(delim);
do {
- pos = in.find_first_of(delim);
- elem = in.substr(0, pos);
- Trim(elem, delim, &trimed_elem);
- if (!trimed_elem.empty()) out->push_back(trimed_elem);
- in = in.substr(pos+1);
- } while (pos != std::string::npos);
+ std::string elem = in.substr(begin, end - begin);
+ if (!elem.empty()) out.push_back(std::move(elem));
Review Comment:
I think it seems more reasonable if `Split("1 2", " ")` returns `{"1", "2"}`, and it is same as previous behavior.
And `Split("1 2","")` returns `{"1", " ", "2"}`.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #644: Refactor Trim/Split/Split2KV functions in util.h
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #644:
URL: https://github.com/apache/incubator-kvrocks/pull/644#discussion_r902440112
##########
src/util.cc:
##########
@@ -371,7 +371,14 @@ std::string Trim(std::string in, const std::string &chars) {
std::vector<std::string> Split(const std::string &in, const std::string &delim) {
std::vector<std::string> out;
- if (in.empty() || delim.empty()) return out;
+ if (in.empty()) {
+ return out;
+ }
+
+ if (delim.empty()) {
Review Comment:
So thanks, I'll fix it.
BTW, this function has some difference than some well known `split`: it split strings regarding to regex "a|b|c|..." where delim="abc...", so I am considering to add another split that split strings just regarding to the provided delimiter.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice merged pull request #644: Refactor Trim/Split/Split2KV functions in util.h
Posted by GitBox <gi...@apache.org>.
PragmaTwice merged PR #644:
URL: https://github.com/apache/incubator-kvrocks/pull/644
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #644: Refactor Trim/Split/Split2KV functions in util.h
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #644:
URL: https://github.com/apache/incubator-kvrocks/pull/644#discussion_r903255268
##########
src/util.cc:
##########
@@ -538,25 +550,29 @@ std::string StringToHex(const std::string &input) {
return output;
}
+constexpr std::uint64_t ExpTo1024(size_t n) {
+ return static_cast<std::uint64_t>(1) << (10 * n);
+}
+
void BytesToHuman(char *buf, size_t size, uint64_t n) {
double d;
- if (n < 1024) {
+ if (n < ExpTo1024(1)) {
Review Comment:
It just replace `1024*1024*...*1024` to something like `1024^n`, so I don't think it will affect readability. And it is not related to `bytes`.
> And this change is not related to theme
Agree. Maybe move it to a separate PR.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #644: Refactor Trim/Split/Split2KV functions in util.h
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #644:
URL: https://github.com/apache/incubator-kvrocks/pull/644#discussion_r903255268
##########
src/util.cc:
##########
@@ -538,25 +550,29 @@ std::string StringToHex(const std::string &input) {
return output;
}
+constexpr std::uint64_t ExpTo1024(size_t n) {
+ return static_cast<std::uint64_t>(1) << (10 * n);
+}
+
void BytesToHuman(char *buf, size_t size, uint64_t n) {
double d;
- if (n < 1024) {
+ if (n < ExpTo1024(1)) {
Review Comment:
It just replace `1024*1024*...*1024` to something like `1024^n`, so I don't think it will affect readability.
> And this change is not related to theme
Agree. Maybe move it to a separate PR.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #644: Refactor Trim/Split/Split2KV functions in util.h
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #644:
URL: https://github.com/apache/incubator-kvrocks/pull/644#issuecomment-1161489673
> Generally looks good. One comment:
>
> It seems that `Trim` will update the input string while `Split` doesn't. This looks inconsistent.
>
> Also, if `Split` with empty delim, we may return the input string as is.
Thanks @tisonkun !
> `Trim` will update the input string while `Split` doesn't
`Trim` takes a copy of the first argument, not a reference.
> if `Split` with empty delim, we may return the input string as is.
done~
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #644: Refactor Trim/Split/Split2KV functions in util.h
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #644:
URL: https://github.com/apache/incubator-kvrocks/pull/644#discussion_r902807971
##########
src/util.cc:
##########
@@ -359,42 +359,54 @@ std::string ToLower(std::string in) {
return in;
}
-void Trim(const std::string &in, const std::string &chars, std::string *out) {
- out->clear();
- if (in.empty()) return;
- out->assign(in);
- out->erase(0, out->find_first_not_of(chars));
- out->erase(out->find_last_not_of(chars)+1);
+std::string Trim(std::string in, const std::string &chars) {
+ if (in.empty()) return in;
+
+ in.erase(0, in.find_first_not_of(chars));
+ in.erase(in.find_last_not_of(chars) + 1);
+
+ return in;
}
-void Split(std::string in, std::string delim, std::vector<std::string> *out) {
- if (in.empty() || !out) return;
- out->clear();
+std::vector<std::string> Split(const std::string &in, const std::string &delim) {
+ std::vector<std::string> out;
+
+ if (in.empty()) {
+ return out;
+ }
+
+ if (delim.empty()) {
+ out.resize(in.size());
+ std::transform(in.begin(), in.end(), out.begin(),
+ [](char c) -> std::string { return {c}; });
+ return out;
+ }
- std::string::size_type pos = 0;
- std::string elem, trimed_elem;
+ size_t begin = 0, end = in.find_first_of(delim);
do {
- pos = in.find_first_of(delim);
- elem = in.substr(0, pos);
- Trim(elem, delim, &trimed_elem);
- if (!trimed_elem.empty()) out->push_back(trimed_elem);
- in = in.substr(pos+1);
- } while (pos != std::string::npos);
+ std::string elem = in.substr(begin, end - begin);
+ if (!elem.empty()) out.push_back(std::move(elem));
Review Comment:
`Split("1 2")` should normally return `{"1", "", "2"}`. But with this condition, it returns `{"1", "2"}`.
I don't know if kvrocks depends on this behavior but point out it's diverged from other libraries.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org