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/09/19 16:27:49 UTC
[GitHub] [incubator-kvrocks] tanruixiang opened a new pull request, #897: Replace std::stol with ParseInt
tanruixiang opened a new pull request, #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897
It closes #896
--
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] tanruixiang commented on a diff in pull request #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974824786
##########
src/redis_cmd.cc:
##########
@@ -820,12 +815,11 @@ class CommandDel : public Commander {
};
Status getBitOffsetFromArgument(std::string arg, uint32_t *offset) {
- int64_t offset_arg = 0;
- try {
- offset_arg = std::stoll(arg);
- } catch (std::exception &e) {
+ auto parseResult = ParseInt<int64_t>(arg, 10);
Review Comment:
That is to say, the type used in the original code shoule be the same here. In this pr, regardless of whether the type can be adjusted logically. If the type can be modified, we can do it in the next 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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974455519
##########
src/redis_cmd.cc:
##########
@@ -488,16 +488,12 @@ class CommandSet : public Commander {
if (ttl_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
} else if (opt == "px" && !ttl_ && !last_arg) {
int64_t ttl_ms = 0;
- try {
- std::string s = args_[++i];
- std::string::size_type sz;
- ttl_ms = std::stol(s, &sz);
- if (sz != s.size()) {
- return Status(Status::RedisParseErr, errValueNotInterger);
- }
- } catch (std::exception &e) {
+ std::string s = args_[++i];
+ auto parseResult = ParseInt<int64_t>(s, /* base= */ 10);
+ if (!parseResult.IsOK()) {
Review Comment:
```suggestion
if (!parseResult) {
```
--
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] tanruixiang commented on a diff in pull request #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r977197665
##########
tests/tcl/tests/unit/type/hash.tcl:
##########
@@ -335,8 +335,8 @@ start_server {tags {"hash"}} {
catch {r hincrby smallhash str 1} smallerr
catch {r hincrby smallhash str 1} bigerr
set rv {}
- lappend rv [string match "ERR*not an integer*" $smallerr]
- lappend rv [string match "ERR*not an integer*" $bigerr]
+ lappend rv [string match "ERR*non-integer*" $smallerr]
+ lappend rv [string match "ERR*non-integer*" $bigerr]
Review Comment:
> why do you need these changes?
Use the `ToStatus()` method of class `StatusOr` to get error information. It is expressed slightly differently than it was hardcoded before.
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#issuecomment-1257141697
Thanks. I will merge it soon.
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974542401
##########
src/redis_cmd.cc:
##########
@@ -1104,20 +1109,20 @@ class CommandPExpire : public Commander {
Status Parse(const std::vector<std::string> &args) override {
int64_t now;
rocksdb::Env::Default()->GetCurrentTime(&now);
- try {
- auto ttl_ms = std::stol(args[2]);
- if (ttl_ms > 0 && ttl_ms < 1000) {
- seconds_ = 1;
- } else {
- seconds_ = ttl_ms / 1000;
- if (seconds_ >= INT32_MAX - now) {
- return Status(Status::RedisParseErr, "the expire time was overflow");
- }
- }
- seconds_ += now;
- } catch (std::exception &e) {
+ auto parseResult = ParseInt<long int>(args[2], 10);
Review Comment:
maybe uint64_t is ok here.
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974544409
##########
src/redis_cmd.cc:
##########
@@ -1517,16 +1522,16 @@ class CommandPop : public Commander {
if (args.size() == 2) {
return Status::OK();
}
- try {
- int32_t v = std::stol(args[2]);
- if (v < 0) {
- return Status(Status::RedisParseErr, errValueMustBePositive);
- }
- count_ = v;
- with_count_ = true;
- } catch (const std::exception& ) {
+ auto parseResult = ParseInt<int32_t>(args[2], 10);
+ if (!parseResult) {
return Status(Status::RedisParseErr, errValueNotInterger);
}
+ int32_t v = *parseResult;
+ if (v < 0) {
+ return Status(Status::RedisParseErr, errValueMustBePositive);
+ }
+ count_ = v;
Review Comment:
`v` is unnecessary
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974826523
##########
src/util.cc:
##########
@@ -341,26 +342,20 @@ int GetLocalPort(int fd) {
}
Status DecimalStringToNum(const std::string &str, int64_t *n, int64_t min, int64_t max) {
- try {
- *n = static_cast<int64_t>(std::stoll(str));
- if (max > min && (*n < min || *n > max)) {
- return Status(Status::NotOK, "value should between "+std::to_string(min)+" and "+std::to_string(max));
- }
- } catch (std::exception &e) {
- return Status(Status::NotOK, "value is not an integer or out of range");
+ auto parseResult = ParseInt<int64_t>(str, NumericRange<int64_t>{min, max}, 10);
+ if (!parseResult) {
+ return Status(Status::NotOK, parseResult.Msg());
}
+ *n = *parseResult;
return Status::OK();
}
Status OctalStringToNum(const std::string &str, int64_t *n, int64_t min, int64_t max) {
- try {
- *n = static_cast<int64_t>(std::stoll(str, nullptr, 8));
- if (max > min && (*n < min || *n > max)) {
- return Status(Status::NotOK, "value should between "+std::to_string(min)+" and "+std::to_string(max));
- }
- } catch (std::exception &e) {
- return Status(Status::NotOK, "value is not an integer or out of range");
+ auto parseResult = ParseInt<int64_t>(str, NumericRange<int64_t>{min, max}, 8);
+ if (!parseResult) {
+ return Status(Status::NotOK, parseResult.Msg());
Review Comment:
```suggestion
return parseResult.ToStatus();
```
--
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] tanruixiang commented on pull request #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#issuecomment-1252262360
> > > That is to say, the type used in the original code shoule be the same here. In this pr, regardless of whether the type can be adjusted logically. If the type can be modified, we can do it in the next Pr?
> >
> >
> > @PragmaTwice HI. What do you think about this. If you want to modify it in this pr, I will modify it immediately, otherwise I will modify it to another pr.
>
> I think it is ok to change it, since it is trivial.
Ok, I will change it .
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r978678205
##########
tests/tcl/tests/unit/type/hash.tcl:
##########
@@ -335,8 +335,8 @@ start_server {tags {"hash"}} {
catch {r hincrby smallhash str 1} smallerr
catch {r hincrby smallhash str 1} bigerr
set rv {}
- lappend rv [string match "ERR*not an integer*" $smallerr]
- lappend rv [string match "ERR*not an integer*" $bigerr]
+ lappend rv [string match "ERR*non-integer*" $smallerr]
+ lappend rv [string match "ERR*non-integer*" $bigerr]
Review Comment:
I think these changes are acceptable. These messages come from `ParseInt`, and are forwarded to these callers.
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974472209
##########
src/redis_cmd.cc:
##########
@@ -5363,13 +5368,12 @@ class CommandXRead : public Commander {
if (i+1 >= args.size()) {
return Status(Status::RedisParseErr, errInvalidSyntax);
}
-
- try {
- with_count_ = true;
- count_ = static_cast<uint64_t>(std::stoll(args[i+1]));
- } catch (const std::exception &) {
+ with_count_ = true;
+ auto parseResult = ParseInt<int64_t>(args[i+1], /* base= */ 10);
+ if (!parseResult.IsOK()) {
return Status(Status::RedisParseErr, errValueNotInterger);
}
+ count_ = static_cast<uint64_t>(parseResult.GetValue());
Review Comment:
use ParseInt<unsigned something>
##########
src/redis_cmd.cc:
##########
@@ -5363,13 +5368,12 @@ class CommandXRead : public Commander {
if (i+1 >= args.size()) {
return Status(Status::RedisParseErr, errInvalidSyntax);
}
-
- try {
- with_count_ = true;
- count_ = static_cast<uint64_t>(std::stoll(args[i+1]));
- } catch (const std::exception &) {
+ with_count_ = true;
+ auto parseResult = ParseInt<int64_t>(args[i+1], /* base= */ 10);
+ if (!parseResult.IsOK()) {
return Status(Status::RedisParseErr, errValueNotInterger);
}
+ count_ = static_cast<uint64_t>(parseResult.GetValue());
Review Comment:
use `ParseInt<unsigned something>`
--
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] tanruixiang commented on a diff in pull request #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974813736
##########
src/redis_cmd.cc:
##########
@@ -1517,16 +1522,16 @@ class CommandPop : public Commander {
if (args.size() == 2) {
return Status::OK();
}
- try {
- int32_t v = std::stol(args[2]);
- if (v < 0) {
- return Status(Status::RedisParseErr, errValueMustBePositive);
- }
- count_ = v;
- with_count_ = true;
- } catch (const std::exception& ) {
+ auto parseResult = ParseInt<int32_t>(args[2], 10);
+ if (!parseResult) {
return Status(Status::RedisParseErr, errValueNotInterger);
}
+ int32_t v = *parseResult;
+ if (v < 0) {
+ return Status(Status::RedisParseErr, errValueMustBePositive);
+ }
+ count_ = v;
Review Comment:
Yes. I think it would be simpler to replace parseResult with v.
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974459823
##########
src/util.cc:
##########
@@ -341,26 +342,26 @@ int GetLocalPort(int fd) {
}
Status DecimalStringToNum(const std::string &str, int64_t *n, int64_t min, int64_t max) {
- try {
- *n = static_cast<int64_t>(std::stoll(str));
- if (max > min && (*n < min || *n > max)) {
- return Status(Status::NotOK, "value should between "+std::to_string(min)+" and "+std::to_string(max));
- }
- } catch (std::exception &e) {
+ auto parseResult = ParseInt<int64_t>(str, /* base= */ 10);
+ if (!parseResult.IsOK()) {
return Status(Status::NotOK, "value is not an integer or out of range");
}
+ *n = parseResult.GetValue();
+ if (max > min && (*n < min || *n > max)) {
+ return Status(Status::NotOK, "value should between "+std::to_string(min)+" and "+std::to_string(max));
+ }
Review Comment:
Use another overload of ParseInt which can accept a range.
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974461758
##########
src/redis_cmd.cc:
##########
@@ -488,16 +488,12 @@ class CommandSet : public Commander {
if (ttl_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
} else if (opt == "px" && !ttl_ && !last_arg) {
int64_t ttl_ms = 0;
- try {
- std::string s = args_[++i];
- std::string::size_type sz;
- ttl_ms = std::stol(s, &sz);
- if (sz != s.size()) {
- return Status(Status::RedisParseErr, errValueNotInterger);
- }
- } catch (std::exception &e) {
+ std::string s = args_[++i];
+ auto parseResult = ParseInt<int64_t>(s, /* base= */ 10);
Review Comment:
not `int64_t`
--
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] tanruixiang commented on a diff in pull request #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974493193
##########
src/redis_cmd.cc:
##########
@@ -488,16 +488,12 @@ class CommandSet : public Commander {
if (ttl_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
} else if (opt == "px" && !ttl_ && !last_arg) {
int64_t ttl_ms = 0;
- try {
- std::string s = args_[++i];
- std::string::size_type sz;
- ttl_ms = std::stol(s, &sz);
- if (sz != s.size()) {
- return Status(Status::RedisParseErr, errValueNotInterger);
- }
- } catch (std::exception &e) {
+ std::string s = args_[++i];
+ auto parseResult = ParseInt<int64_t>(s, /* base= */ 10);
Review Comment:
Hi. This type is int64_t.
--
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] tanruixiang commented on pull request #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#issuecomment-1251336776
@PragmaTwice Hi. The original code uses std:: stol, but using ParseInt<long int>instead the lint will report an error. Do you have any better suggestions?
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974824071
##########
src/redis_cmd.cc:
##########
@@ -889,12 +883,16 @@ class CommandBitCount : public Commander {
return Status(Status::RedisParseErr, errInvalidSyntax);
}
if (args.size() == 4) {
- try {
- start_ = std::stol(args[2]);
- stop_ = std::stol(args[3]);
- } catch (std::exception &e) {
+ auto parseArgs2Result = ParseInt<int64_t>(args[2], 10);
+ if (!parseArgs2Result) {
+ return Status(Status::RedisParseErr, errValueNotInterger);
+ }
+ start_ = *parseArgs2Result;
+ auto parseArgs3Result = ParseInt<int64_t>(args[3], 10);
Review Comment:
these variable names are weird
--
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] tanruixiang commented on a diff in pull request #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974813736
##########
src/redis_cmd.cc:
##########
@@ -1517,16 +1522,16 @@ class CommandPop : public Commander {
if (args.size() == 2) {
return Status::OK();
}
- try {
- int32_t v = std::stol(args[2]);
- if (v < 0) {
- return Status(Status::RedisParseErr, errValueMustBePositive);
- }
- count_ = v;
- with_count_ = true;
- } catch (const std::exception& ) {
+ auto parseResult = ParseInt<int32_t>(args[2], 10);
+ if (!parseResult) {
return Status(Status::RedisParseErr, errValueNotInterger);
}
+ int32_t v = *parseResult;
+ if (v < 0) {
+ return Status(Status::RedisParseErr, errValueMustBePositive);
+ }
+ count_ = v;
Review Comment:
Yes. I think it would be more simpler to replace parseResult with v.
--
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] tanruixiang commented on pull request #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#issuecomment-1251282691
@PragmaTwice Thanks for your warm help. I'm just going to upload a draft to save my progress and keep doing it tomorrow. I didn't even check it myself. You have helped me. Thank you very much😆
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974823434
##########
src/redis_cmd.cc:
##########
@@ -820,12 +815,11 @@ class CommandDel : public Commander {
};
Status getBitOffsetFromArgument(std::string arg, uint32_t *offset) {
- int64_t offset_arg = 0;
- try {
- offset_arg = std::stoll(arg);
- } catch (std::exception &e) {
+ auto parseResult = ParseInt<int64_t>(arg, 10);
Review Comment:
If you parse a int64_t, and then check whether its range is exactly in (0, UINT32_MAX), you can just parse a uint32_t instead.
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#issuecomment-1251863098
> > That is to say, the type used in the original code shoule be the same here. In this pr, regardless of whether the type can be adjusted logically. If the type can be modified, we can do it in the next Pr?
>
> @PragmaTwice HI. What do you think about this. If you want to modify it in this pr, I will modify it immediately, otherwise I will modify it to another pr.
I think it is ok to change it, since it is trivial.
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974546053
##########
src/redis_cmd.cc:
##########
@@ -1517,16 +1522,16 @@ class CommandPop : public Commander {
if (args.size() == 2) {
return Status::OK();
}
- try {
- int32_t v = std::stol(args[2]);
- if (v < 0) {
- return Status(Status::RedisParseErr, errValueMustBePositive);
- }
- count_ = v;
- with_count_ = true;
- } catch (const std::exception& ) {
+ auto parseResult = ParseInt<int32_t>(args[2], 10);
+ if (!parseResult) {
return Status(Status::RedisParseErr, errValueNotInterger);
}
+ int32_t v = *parseResult;
+ if (v < 0) {
+ return Status(Status::RedisParseErr, errValueMustBePositive);
+ }
+ count_ = v;
Review Comment:
```suggestion
auto v = ParseInt<int32_t>(args[2], 10);
if (!v) {
return Status(Status::RedisParseErr, errValueNotInterger);
}
if (*v < 0) {
return Status(Status::RedisParseErr, errValueMustBePositive);
}
count_ = *v;
```
--
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] tanruixiang commented on a diff in pull request #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974813999
##########
src/redis_cmd.cc:
##########
@@ -1104,20 +1109,20 @@ class CommandPExpire : public Commander {
Status Parse(const std::vector<std::string> &args) override {
int64_t now;
rocksdb::Env::Default()->GetCurrentTime(&now);
- try {
- auto ttl_ms = std::stol(args[2]);
- if (ttl_ms > 0 && ttl_ms < 1000) {
- seconds_ = 1;
- } else {
- seconds_ = ttl_ms / 1000;
- if (seconds_ >= INT32_MAX - now) {
- return Status(Status::RedisParseErr, "the expire time was overflow");
- }
- }
- seconds_ += now;
- } catch (std::exception &e) {
+ auto parseResult = ParseInt<long int>(args[2], 10);
Review Comment:
This is same above.
--
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] tanruixiang commented on a diff in pull request #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974482423
##########
src/redis_cmd.cc:
##########
@@ -889,12 +884,16 @@ class CommandBitCount : public Commander {
return Status(Status::RedisParseErr, errInvalidSyntax);
}
if (args.size() == 4) {
- try {
- start_ = std::stol(args[2]);
- stop_ = std::stol(args[3]);
- } catch (std::exception &e) {
+ auto parseArgs2Result = ParseInt<int64_t>(args[2], /* base= */ 10);
Review Comment:
>
Hi. This type is int64_t.
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974468714
##########
tests/cppunit/t_hash_test.cc:
##########
@@ -114,7 +115,11 @@ TEST_F(RedisHashTest, HIncr) {
}
std::string bytes;
hash->Get(key_, field, &bytes);
- value = std::stoll(bytes);
+ auto parseResult = ParseInt<int64_t>(bytes, /* base= */ 10);
+ if (!parseResult.IsOK()) {
+ EXPECT_TRUE(false);
Review Comment:
```suggestion
FAIL();
```
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974823725
##########
src/redis_cmd.cc:
##########
@@ -820,12 +815,11 @@ class CommandDel : public Commander {
};
Status getBitOffsetFromArgument(std::string arg, uint32_t *offset) {
- int64_t offset_arg = 0;
- try {
- offset_arg = std::stoll(arg);
- } catch (std::exception &e) {
+ auto parseResult = ParseInt<int64_t>(arg, 10);
Review Comment:
BTW, you can just return the original status.
```
return res.ToStatus();
```
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974461758
##########
src/redis_cmd.cc:
##########
@@ -488,16 +488,12 @@ class CommandSet : public Commander {
if (ttl_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
} else if (opt == "px" && !ttl_ && !last_arg) {
int64_t ttl_ms = 0;
- try {
- std::string s = args_[++i];
- std::string::size_type sz;
- ttl_ms = std::stol(s, &sz);
- if (sz != s.size()) {
- return Status(Status::RedisParseErr, errValueNotInterger);
- }
- } catch (std::exception &e) {
+ std::string s = args_[++i];
+ auto parseResult = ParseInt<int64_t>(s, /* base= */ 10);
Review Comment:
not `int64_t`, be careful with integer types.
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974458299
##########
src/redis_cmd.cc:
##########
@@ -820,12 +816,11 @@ class CommandDel : public Commander {
};
Status getBitOffsetFromArgument(std::string arg, uint32_t *offset) {
- int64_t offset_arg = 0;
- try {
- offset_arg = std::stoll(arg);
- } catch (std::exception &e) {
+ auto parseResult = ParseInt<int64_t>(arg, /* base= */ 10);
+ if (!parseResult.IsOK()) {
return Status(Status::RedisParseErr, errValueNotInterger);
}
+ int64_t offset_arg = offset_arg = parseResult.GetValue();
Review Comment:
```suggestion
int64_t offset_arg = *parseResult;
```
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974462778
##########
src/redis_cmd.cc:
##########
@@ -564,17 +560,17 @@ class CommandSetEX : public Commander {
class CommandPSetEX : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- try {
- auto ttl_ms = std::stol(args[2]);
- if (ttl_ms <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
- if (ttl_ms > 0 && ttl_ms < 1000) {
- ttl_ = 1;
- } else {
- ttl_ = ttl_ms / 1000;
- }
- } catch (std::exception &e) {
+ auto parseResult = ParseInt<int64_t>(args[2], /* base= */ 10);
Review Comment:
not `int64_t`
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974472209
##########
src/redis_cmd.cc:
##########
@@ -5363,13 +5368,12 @@ class CommandXRead : public Commander {
if (i+1 >= args.size()) {
return Status(Status::RedisParseErr, errInvalidSyntax);
}
-
- try {
- with_count_ = true;
- count_ = static_cast<uint64_t>(std::stoll(args[i+1]));
- } catch (const std::exception &) {
+ with_count_ = true;
+ auto parseResult = ParseInt<int64_t>(args[i+1], /* base= */ 10);
+ if (!parseResult.IsOK()) {
return Status(Status::RedisParseErr, errValueNotInterger);
}
+ count_ = static_cast<uint64_t>(parseResult.GetValue());
Review Comment:
you can use `ParseInt<unsigned something>`
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974464040
##########
src/redis_cmd.cc:
##########
@@ -1104,20 +1109,20 @@ class CommandPExpire : public Commander {
Status Parse(const std::vector<std::string> &args) override {
int64_t now;
rocksdb::Env::Default()->GetCurrentTime(&now);
- try {
- auto ttl_ms = std::stol(args[2]);
- if (ttl_ms > 0 && ttl_ms < 1000) {
- seconds_ = 1;
- } else {
- seconds_ = ttl_ms / 1000;
- if (seconds_ >= INT32_MAX - now) {
- return Status(Status::RedisParseErr, "the expire time was overflow");
- }
- }
- seconds_ += now;
- } catch (std::exception &e) {
+ auto parseResult = ParseInt<int64_t>(args[2], /* base= */ 10);
Review Comment:
you can just remove `/* base= */`
--
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] tanruixiang commented on a diff in pull request #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974824114
##########
src/redis_cmd.cc:
##########
@@ -820,12 +815,11 @@ class CommandDel : public Commander {
};
Status getBitOffsetFromArgument(std::string arg, uint32_t *offset) {
- int64_t offset_arg = 0;
- try {
- offset_arg = std::stoll(arg);
- } catch (std::exception &e) {
+ auto parseResult = ParseInt<int64_t>(arg, 10);
Review Comment:
> If you parse a int64_t, and then check whether its range is exactly in (0, UINT32_MAX), you can just parse a uint32_t instead.
yes I know what you mean. I mean, should this pr be defined as trying to restore the original code.
--
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] tanruixiang commented on a diff in pull request #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974821665
##########
src/redis_cmd.cc:
##########
@@ -820,12 +815,11 @@ class CommandDel : public Commander {
};
Status getBitOffsetFromArgument(std::string arg, uint32_t *offset) {
- int64_t offset_arg = 0;
- try {
- offset_arg = std::stoll(arg);
- } catch (std::exception &e) {
+ auto parseResult = ParseInt<int64_t>(arg, 10);
Review Comment:
Hi. I think that in this simple pr, we do not need to modify the data type according to logic, and it is important to restore the original data type as much as possible. I think we can reopen a PR to make these logical adjustments. What do you think.
##########
src/redis_cmd.cc:
##########
@@ -820,12 +815,11 @@ class CommandDel : public Commander {
};
Status getBitOffsetFromArgument(std::string arg, uint32_t *offset) {
- int64_t offset_arg = 0;
- try {
- offset_arg = std::stoll(arg);
- } catch (std::exception &e) {
+ auto parseResult = ParseInt<int64_t>(arg, 10);
Review Comment:
@PragmaTwice Hi. I think that in this simple pr, we do not need to modify the data type according to logic, and it is important to restore the original data type as much as possible. I think we can reopen a PR to make these logical adjustments. What do you think.
--
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] tanruixiang commented on a diff in pull request #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974813289
##########
src/redis_cmd.cc:
##########
@@ -564,17 +560,17 @@ class CommandSetEX : public Commander {
class CommandPSetEX : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- try {
- auto ttl_ms = std::stol(args[2]);
- if (ttl_ms <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
- if (ttl_ms > 0 && ttl_ms < 1000) {
- ttl_ = 1;
- } else {
- ttl_ = ttl_ms / 1000;
- }
- } catch (std::exception &e) {
+ auto parseResult = ParseInt<long int>(args[2], 10);
Review Comment:
Maybe we can just use int64_t, because the next code will check the range.
`
if (ttl_ms > 0 && ttl_ms < 1000) {
ttl_ = 1;
} else {
ttl_ = ttl_ms / 1000;
}
`
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#issuecomment-1256982703
> > Hi @tanruixiang, I notice there is also some code using `std::stoul` or `std::stoull` in kvrocks, do you want to also replace them in this PR? Or it is OK to merge it and replace them in another PR.
>
> OK I am willing to do this task. I think this PR information is a bit too much, we can continue to do this in the next PR.
Fine. Maybe there are also some `atoi` and `std::stoi`, you can replace them too.
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974463415
##########
src/redis_cmd.cc:
##########
@@ -889,12 +884,16 @@ class CommandBitCount : public Commander {
return Status(Status::RedisParseErr, errInvalidSyntax);
}
if (args.size() == 4) {
- try {
- start_ = std::stol(args[2]);
- stop_ = std::stol(args[3]);
- } catch (std::exception &e) {
+ auto parseArgs2Result = ParseInt<int64_t>(args[2], /* base= */ 10);
Review Comment:
same as above
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974535993
##########
src/redis_cmd.cc:
##########
@@ -564,17 +560,17 @@ class CommandSetEX : public Commander {
class CommandPSetEX : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- try {
- auto ttl_ms = std::stol(args[2]);
- if (ttl_ms <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
- if (ttl_ms > 0 && ttl_ms < 1000) {
- ttl_ = 1;
- } else {
- ttl_ = ttl_ms / 1000;
- }
- } catch (std::exception &e) {
+ auto parseResult = ParseInt<long int>(args[2], 10);
Review Comment:
```suggestion
auto parseResult = ParseInt<int>(args[2], 10);
```
`ttl_` is typed `int`
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974824071
##########
src/redis_cmd.cc:
##########
@@ -889,12 +883,16 @@ class CommandBitCount : public Commander {
return Status(Status::RedisParseErr, errInvalidSyntax);
}
if (args.size() == 4) {
- try {
- start_ = std::stol(args[2]);
- stop_ = std::stol(args[3]);
- } catch (std::exception &e) {
+ auto parseArgs2Result = ParseInt<int64_t>(args[2], 10);
+ if (!parseArgs2Result) {
+ return Status(Status::RedisParseErr, errValueNotInterger);
+ }
+ start_ = *parseArgs2Result;
+ auto parseArgs3Result = ParseInt<int64_t>(args[3], 10);
Review Comment:
just `arg_stop`, `parse_stop`, `res` or something like that is ok
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974820360
##########
src/redis_cmd.cc:
##########
@@ -820,12 +815,11 @@ class CommandDel : public Commander {
};
Status getBitOffsetFromArgument(std::string arg, uint32_t *offset) {
- int64_t offset_arg = 0;
- try {
- offset_arg = std::stoll(arg);
- } catch (std::exception &e) {
+ auto parseResult = ParseInt<int64_t>(arg, 10);
Review Comment:
uint32_t
--
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] tanruixiang commented on pull request #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#issuecomment-1251836912
> That is to say, the type used in the original code shoule be the same here. In this pr, regardless of whether the type can be adjusted logically. If the type can be modified, we can do it in the next Pr?
HI. What do you think about this. If you want to modify it in this pr, I will modify it immediately, otherwise I will modify it to another 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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974545242
##########
src/redis_cmd.cc:
##########
@@ -1517,16 +1522,16 @@ class CommandPop : public Commander {
if (args.size() == 2) {
return Status::OK();
}
- try {
- int32_t v = std::stol(args[2]);
- if (v < 0) {
- return Status(Status::RedisParseErr, errValueMustBePositive);
- }
- count_ = v;
- with_count_ = true;
- } catch (const std::exception& ) {
+ auto parseResult = ParseInt<int32_t>(args[2], 10);
+ if (!parseResult) {
return Status(Status::RedisParseErr, errValueNotInterger);
}
+ int32_t v = *parseResult;
Review Comment:
actually we prefer underscore-case for local variable, and for here, you can use `v` instead of `parseResult`
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice merged PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897
--
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] tanruixiang commented on a diff in pull request #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974810233
##########
src/redis_cmd.cc:
##########
@@ -564,17 +560,17 @@ class CommandSetEX : public Commander {
class CommandPSetEX : public Commander {
public:
Status Parse(const std::vector<std::string> &args) override {
- try {
- auto ttl_ms = std::stol(args[2]);
- if (ttl_ms <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
- if (ttl_ms > 0 && ttl_ms < 1000) {
- ttl_ = 1;
- } else {
- ttl_ = ttl_ms / 1000;
- }
- } catch (std::exception &e) {
+ auto parseResult = ParseInt<long int>(args[2], 10);
Review Comment:
`Long int` and `int` are not equal on some machines. Do you have any good suggestions about this?
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974469097
##########
tests/cppunit/t_hash_test.cc:
##########
@@ -114,7 +115,11 @@ TEST_F(RedisHashTest, HIncr) {
}
std::string bytes;
hash->Get(key_, field, &bytes);
- value = std::stoll(bytes);
+ auto parseResult = ParseInt<int64_t>(bytes, /* base= */ 10);
+ if (!parseResult.IsOK()) {
+ EXPECT_TRUE(false);
+ }
+ value = parseResult.GetValue();
EXPECT_EQ(32, value);
Review Comment:
```suggestion
EXPECT_EQ(32, *parseResult);
```
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974820360
##########
src/redis_cmd.cc:
##########
@@ -820,12 +815,11 @@ class CommandDel : public Commander {
};
Status getBitOffsetFromArgument(std::string arg, uint32_t *offset) {
- int64_t offset_arg = 0;
- try {
- offset_arg = std::stoll(arg);
- } catch (std::exception &e) {
+ auto parseResult = ParseInt<int64_t>(arg, 10);
Review Comment:
uint33_t
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974459823
##########
src/util.cc:
##########
@@ -341,26 +342,26 @@ int GetLocalPort(int fd) {
}
Status DecimalStringToNum(const std::string &str, int64_t *n, int64_t min, int64_t max) {
- try {
- *n = static_cast<int64_t>(std::stoll(str));
- if (max > min && (*n < min || *n > max)) {
- return Status(Status::NotOK, "value should between "+std::to_string(min)+" and "+std::to_string(max));
- }
- } catch (std::exception &e) {
+ auto parseResult = ParseInt<int64_t>(str, /* base= */ 10);
+ if (!parseResult.IsOK()) {
return Status(Status::NotOK, "value is not an integer or out of range");
}
+ *n = parseResult.GetValue();
+ if (max > min && (*n < min || *n > max)) {
+ return Status(Status::NotOK, "value should between "+std::to_string(min)+" and "+std::to_string(max));
+ }
Review Comment:
you can use another overload of ParseInt which can accept a range.
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r974460791
##########
src/redis_cmd.cc:
##########
@@ -488,16 +488,12 @@ class CommandSet : public Commander {
if (ttl_ <= 0) return Status(Status::RedisParseErr, errInvalidExpireTime);
} else if (opt == "px" && !ttl_ && !last_arg) {
int64_t ttl_ms = 0;
- try {
- std::string s = args_[++i];
- std::string::size_type sz;
- ttl_ms = std::stol(s, &sz);
- if (sz != s.size()) {
- return Status(Status::RedisParseErr, errValueNotInterger);
- }
- } catch (std::exception &e) {
+ std::string s = args_[++i];
+ auto parseResult = ParseInt<int64_t>(s, /* base= */ 10);
Review Comment:
Actually the `<int64_t>` can be omitted.
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#discussion_r977195199
##########
tests/tcl/tests/unit/type/hash.tcl:
##########
@@ -335,8 +335,8 @@ start_server {tags {"hash"}} {
catch {r hincrby smallhash str 1} smallerr
catch {r hincrby smallhash str 1} bigerr
set rv {}
- lappend rv [string match "ERR*not an integer*" $smallerr]
- lappend rv [string match "ERR*not an integer*" $bigerr]
+ lappend rv [string match "ERR*non-integer*" $smallerr]
+ lappend rv [string match "ERR*non-integer*" $bigerr]
Review Comment:
why do you need these changes?
--
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 #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#issuecomment-1256929672
Hi @tanruixiang, I notice there is also some code using `std::stoul` or `std::stoull` in kvrocks, do you want to also replace them in this PR? Or it is OK to merge it and replace them in another 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] tanruixiang commented on pull request #897: Replace std::stol with ParseInt
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on PR #897:
URL: https://github.com/apache/incubator-kvrocks/pull/897#issuecomment-1256980526
> Hi @tanruixiang, I notice there is also some code using `std::stoul` or `std::stoull` in kvrocks, do you want to also replace them in this PR? Or it is OK to merge it and replace them in another PR.
OK I am willing to do this task. I think this PR information is a bit too much, we can continue to do this in the next 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