-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: modify cmdId assignment time to assgining after registering cmdtable #2692
fix: modify cmdId assignment time to assgining after registering cmdtable #2692
Conversation
相比于加一个接口,把 cmdId 改为原子变量更好点吧,最小改动原则。这个也不会影响性能。 |
Rather than adding an interface, it would be better to change cmdId to an atomic variable, based on the principle of minimal changes. This will not affect performance either. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
在命令复制那里, 对应的ID有没有复制过去
@@ -34,7 +34,7 @@ class PikaCmdTableManager { | |||
std::shared_ptr<Cmd> GetCmd(const std::string& opt); | |||
bool CmdExist(const std::string& cmd) const; | |||
CmdTable* GetCmdTable(); | |||
uint32_t GetCmdId(); | |||
uint32_t GetMaxCmdId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里为什么要改成 GetMaxCmdId
而且这个函数应该是没用了吧
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个函数确实没用了,当时想着之后可能有获取全局 cmdId 的需求就保留下来。改名是因为此时获取cmdId 是全局最大的 cmdId,所以改下名字更加直观。
在 PikaCmdTableManager::NewCommand 中会调用 cmd 的 Clone() 函数进行命令复制。 Cmd* cmd = GetCmdFromDB(opt, *cmds_);
if (cmd) {
return std::shared_ptr<Cmd>(cmd->Clone());
}
return nullptr; 这里的 Clone() 是 Cmd 基类定义的虚函数,cmdId 是 Cmd基类的成员变量。 一般 Cmd 派生类会重写 Clone() 函数,函数中调用的是 Cmd 派生类的拷贝构造函数。例如下面 ZaddCmd 实现的 Clone() 函数。 Cmd* Clone() override { return new ZAddCmd(*this); }
刚看了下 pika 所有的 Cmd 派生类,在自定义拷贝构造的时候都调用了基类的拷贝构造函数,所以都能保证 cmdId 的成功复制。 |
OK |
…able (#2692) * change cmdId assignment time to assign after intializing cmdtable * 修改 PikaCmdTableManager中 getCmdId,删除pika_command.cc中多余的头文件引用
* fix: modify cmdId assignment time to assgining after registering cmdtable (#2692) * change cmdId assignment time to assign after intializing cmdtable * 修改 PikaCmdTableManager中 getCmdId,删除pika_command.cc中多余的头文件引用 * fix acl bug --------- Co-authored-by: Kaijie Gu <[email protected]> Co-authored-by: liuyuecai <[email protected]>
* fix: modify cmdId assignment time to assgining after registering cmdtable (#2692) * change cmdId assignment time to assign after intializing cmdtable * 修改 PikaCmdTableManager中 getCmdId,删除pika_command.cc中多余的头文件引用 * fix acl bug --------- Co-authored-by: Kaijie Gu <[email protected]> Co-authored-by: liuyuecai <[email protected]>
…able (#2692) * change cmdId assignment time to assign after intializing cmdtable * 修改 PikaCmdTableManager中 getCmdId,删除pika_command.cc中多余的头文件引用
…able (OpenAtomFoundation#2692) * change cmdId assignment time to assign after intializing cmdtable * 修改 PikaCmdTableManager中 getCmdId,删除pika_command.cc中多余的头文件引用
* fix: modify cmdId assignment time to assgining after registering cmdtable (OpenAtomFoundation#2692) * change cmdId assignment time to assign after intializing cmdtable * 修改 PikaCmdTableManager中 getCmdId,删除pika_command.cc中多余的头文件引用 * fix acl bug --------- Co-authored-by: Kaijie Gu <[email protected]> Co-authored-by: liuyuecai <[email protected]>
* fix: modify cmdId assignment time to assgining after registering cmdtable (OpenAtomFoundation#2692) * change cmdId assignment time to assign after intializing cmdtable * 修改 PikaCmdTableManager中 getCmdId,删除pika_command.cc中多余的头文件引用 * fix acl bug --------- Co-authored-by: Kaijie Gu <[email protected]> Co-authored-by: liuyuecai <[email protected]>
Fix #2686
主要解决以下问题
解决方式