[GitHub] [incubator-kvrocks] caipengbo commented on a diff in pull request #921: Use ParseInt to replace stoul, stoull, atoi and stoi

[GitBox <gi...@apache.org>]

caipengbo commented on code in PR #921:
URL: https://github.com/apache/incubator-kvrocks/pull/921#discussion_r979497645


##########
src/config.cc:
##########
@@ -357,10 +358,11 @@ void Config::initFieldCallback() {
         if (args.size() != 2) return Status(Status::NotOK, "wrong number of arguments");
         if (args[0] != "no" && args[1] != "one") {
           master_host = args[0];
-          master_port = std::atoi(args[1].c_str());
-          if (master_port <= 0 || master_port >= 65535) {
+          auto parse_result = ParseInt<int>(args[1].c_str(), NumericRange<int>{1, 65535 - 1}, 10);

Review Comment:
   Extract `65535` as a constant by the way?



##########
src/cluster.cc:
##########
@@ -564,10 +566,11 @@ Status Cluster::ParseClusterNodes(const std::string &nodes_str, ClusterNodes *no
       int start, stop;
       std::vector<std::string> ranges = Util::Split(fields[i], "-");
       if (ranges.size() == 1) {
-        start = std::atoi(ranges[0].c_str());
-        if (IsValidSlot(start) == false) {
+        auto parse_result = ParseInt<int>(ranges[0].c_str(), NumericRange<int>{0, kClusterSlots - 1}, 10);

Review Comment:
   
   Declare `NumericRange<int>{0, kClusterSlots - 1}` as a variable? It appears 3 times in this function.



##########
src/cluster.cc:
##########
@@ -577,9 +580,14 @@ Status Cluster::ParseClusterNodes(const std::string &nodes_str, ClusterNodes *no
           }
         }
       } else if (ranges.size() == 2) {
-        start = std::atoi(ranges[0].c_str());
-        stop = std::atoi(ranges[1].c_str());
-        if (start >= stop || start < 0 || stop >= kClusterSlots) {
+        auto parse_start = ParseInt<int>(ranges[0].c_str(), NumericRange<int>{0, kClusterSlots - 1}, 10);
+        auto parse_stop = ParseInt<int>(ranges[1].c_str(), NumericRange<int>{0, kClusterSlots - 1}, 10);
+        if (!parse_start || !parse_stop) {
+          return Status(Status::ClusterInvalidInfo, "Invalid cluste slot range");
+        }
+        start = *parse_start;
+        stop = *parse_stop;
+        if (start >= stop) {
           return Status(Status::ClusterInvalidInfo, "Invalid cluste slot range");
         }

Review Comment:
   ```suggestion
           if (!parse_start || !parse_stop || *parse_start >= *parse_stop) {
             return Status(Status::ClusterInvalidInfo, "Invalid cluste slot range");
           }
           start = *parse_start;
           stop = *parse_stop;
   ```



-- 
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