[GitHub] [kvrocks] HolyLow commented on a diff in pull request #1737: Add the support of LMPOP command

["HolyLow (via GitHub)" <gi...@apache.org>]

HolyLow commented on code in PR #1737:
URL: https://github.com/apache/kvrocks/pull/1737#discussion_r1315300509


##########
src/commands/cmd_list.cc:
##########
@@ -167,11 +167,11 @@ class CommandLMPop : public Commander {
     }
     num_keys_ = *v;
 
-    if ((args.size() != 3 + num_keys_) && (args.size() != 5 + num_keys_)) {
+    if ((args.size() != num_keys_ + 3) && (args.size() != num_keys_ + 5)) {
       return {Status::RedisParseErr, errWrongNumOfArguments};
     }
 
-    std::string left_or_right = util::ToLower(args[1 + num_keys_]);
+    std::string left_or_right = util::ToLower(args[num_keys_ + 2]);

Review Comment:
   @git-hulk I have some questions on the usage of CommandParser.
   The common usage of CommandParser is like:
   `
   while (cmdParser.isGood()) {
     if (eatCase0()) { doSomething(); }
     else if (eatCase1()) { doSomething(); }
     else { return ParseError(kInvalidSyntax); }
   }
   `
   But this schema assumes no strict order constraint and no strict repeat constraint. For example, the standard LMPOP command is like: 
   
   *LMPOP 2 key1 key2 LEFT COUNT 2*; 
   
   but with the parsing schema above, this command is also considered as valid: 
   
   *LMPOP 2 key1 key2 LEFT RIGHT COUNT 2 COUNT 4 RIGHT*. 
   
   The repeat and the order of keywords are not constrained by the isGood() loop.  Even though we could constrain the repeat by checking args.size(), the order of keywords is not constrained.
   
   So should I constrain the order of the keywords, or leave it loose? If the orders of the keywords SHOULD be constrained, the existing parsers using CommandParser might have parsing bugs, as the CommandParser has no order semantic by design.



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