chore: reduce boilerplate by removing redundant function declarations #6207
Conversation
romange
requested review from
Copilot and
dranikpg
and removed request for
Copilot
Signed-off-by: Roman Gershman <[email protected]>
|
augment review |
There was a problem hiding this comment.
Pull request overview
This PR removes redundant private function declarations from header files for ZSetFamily and StringFamily, and converts command handler static member functions to free functions prefixed with Cmd. The refactoring reduces boilerplate by eliminating the need to declare private command handlers in the header files while maintaining the same functionality through the HFUNC macro expansion pattern.
Key changes:
- Removed private function declarations for command handlers from
zset_family.handstring_family.h - Converted command handler static member functions from
ZSetFamily::CommandNameto free functionsCmdCommandNameinzset_family.cc - Converted command handler static member functions from
StringFamily::CommandNameto free functionsCmdCommandNameinstring_family.cc - Updated HFUNC macros to use
&Cmd##xpattern instead of&ZSetFamily::xor&StringFamily::x - Renamed internal helper function
SetRangetoSetRangeInternalto avoid naming conflict with the newCmdSetRangefree function - Added explicit
ZSetFamily::qualifications for calls to public static methods that remain in the class
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/server/zset_family.h | Removed 38 private command handler function declarations |
| src/server/zset_family.cc | Converted command handlers to free functions (CmdZAdd, CmdZCard, etc.), updated HFUNC macro, added ZSetFamily:: qualifications for public static method calls |
| src/server/string_family.h | Removed 25 private command handler function declarations |
| src/server/string_family.cc | BUG: Register function incorrectly named as CmdRegister instead of StringFamily::Register; converted command handlers to free functions (CmdSet, CmdGet, etc.), updated HFUNC macro, renamed SetRange to SetRangeInternal |
| #define HFUNC(x) SetHandler(&StringFamily::x) | ||
| #define HFUNC(x) SetHandler(&Cmd##x) | ||
|
|
||
| void StringFamily::Register(CommandRegistry* registry) { |
There was a problem hiding this comment.
The function name should be StringFamily::Register to match the declaration in the header file (string_family.h:20), not CmdRegister. This will cause a linker error because StringFamily::Register is declared but not defined, while CmdRegister is defined but never called. The function is invoked as StringFamily::Register(®istry_) in main_service.cc:3060.
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| } | ||
|
|
||
| void StringFamily::Set(CmdArgList args, const CommandContext& cmnd_cntx) { | ||
| void CmdSet(CmdArgList args, const CommandContext& cmnd_cntx) { |
There was a problem hiding this comment.
These Cmd* handlers are now free functions outside the existing anonymous namespace, so they get external linkage. Consider moving them into the anonymous namespace (or making them static) to avoid exporting many Cmd* symbols and potential ODR/link collisions (also applies to the other Cmd* handlers in this file).
🤖 Was this useful? React with 👍 or 👎
| } | ||
|
|
||
| void ZSetFamily::BZPopMin(CmdArgList args, const CommandContext& cmd_cntx) { | ||
| void CmdBZPopMin(CmdArgList args, const CommandContext& cmd_cntx) { |
There was a problem hiding this comment.
These Cmd* command handlers are now file-scope free functions outside the anonymous namespace, so they get external linkage. Consider putting them in the anonymous namespace (or making them static) to prevent leaking lots of Cmd* symbols and reduce the risk of ODR/link collisions (also applies to the other Cmd* handlers in this file).
🤖 Was this useful? React with 👍 or 👎
|
@dranikpg I removed from two families if you are positive with this code removal - I will proceed with others. |
No description provided.