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/27 03:27:18 UTC
[GitHub] [incubator-kvrocks] jackwener opened a new pull request, #683: Remove unique_ptr factory
jackwener opened a new pull request, #683:
URL: https://github.com/apache/incubator-kvrocks/pull/683
Factory is redundant. We don't need to it make unique_ptr with 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 #683: Remove unique_ptr factory
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #683:
URL: https://github.com/apache/incubator-kvrocks/pull/683#discussion_r906950515
##########
src/redis_cmd.cc:
##########
@@ -4714,9 +4714,7 @@ class CommandScript : public Commander {
};
#define ADD_CMD(name, arity, description , first_key, last_key, key_step, fn) \
-{name, arity, description, 0, first_key, last_key, key_step, []() -> std::unique_ptr<Commander> { \
- return std::unique_ptr<Commander>(new fn()); \
-}}
+{name, arity, description, 0, first_key, last_key, key_step}
Review Comment:
Why did you remove `fn`? I think this is a classic factory pattern.
--
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 #683: Remove unique_ptr factory
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #683:
URL: https://github.com/apache/incubator-kvrocks/pull/683#discussion_r906951282
##########
src/redis_cmd.cc:
##########
@@ -4714,9 +4714,7 @@ class CommandScript : public Commander {
};
#define ADD_CMD(name, arity, description , first_key, last_key, key_step, fn) \
-{name, arity, description, 0, first_key, last_key, key_step, []() -> std::unique_ptr<Commander> { \
- return std::unique_ptr<Commander>(new fn()); \
-}}
+{name, arity, description, 0, first_key, last_key, key_step}
Review Comment:
The role of the factory is to dispatch from the command name to the corresponding derived class of `Commander`
--
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] jackwener closed pull request #683: Remove unique_ptr factory
Posted by GitBox <gi...@apache.org>.
jackwener closed pull request #683: Remove unique_ptr factory
URL: https://github.com/apache/incubator-kvrocks/pull/683
--
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