chore: introduce ParsedArguments class #6169
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new ParsedArgs class to provide a cleaner, more explicit API for handling command arguments during dispatch, replacing the generic ArgSlice type at interface boundaries. The change improves code readability by making it clearer when arguments have been parsed and are ready for command dispatch, as opposed to being raw argument slices.
Key changes:
- Introduced
facade::ParsedArgswrapper class with methodsFront(),Tail(),ToSlice(),size(), andempty() - Updated
DispatchCommand()andDispatchManyCommands()interfaces to acceptParsedArgsinstead ofArgSlice - Converted arguments to
ArgSlice(viaToSlice()) only when executing commands, maintaining clear separation between parsing and execution phases
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/facade/facade_types.h | Added new ParsedArgs wrapper class with implicit constructor from ArgSlice |
| src/facade/service_interface.h | Updated virtual interface methods to use ParsedArgs instead of ArgSlice |
| src/facade/command_id.h | Removed unused CmdArgList typedef and facade_types.h include to reduce coupling |
| src/facade/cmd_arg_parser.h | Changed constructor and Tail() return type from CmdArgList to ArgSlice; made HasError() const |
| src/facade/ok_main.cc | Updated OkService implementations to use ParsedArgs parameter type |
| src/facade/dragonfly_connection.cc | Updated dispatch calls to use brace-initialization for ParsedArgs construction |
| src/server/command_registry.h | Updated FindExtended() to accept and return ParsedArgs instead of ArgSlice; added CmdArgList alias |
| src/server/command_registry.cc | Updated FindExtended() implementation to use ParsedArgs API methods |
| src/server/main_service.h | Updated service method signatures to use ParsedArgs |
| src/server/main_service.cc | Converted to use ParsedArgs API throughout dispatch logic; calls ToSlice() when passing to command execution |
| src/server/rdb_load.cc | Updated DispatchCommand() call to use brace-initialization |
| src/server/http_api.cc | Updated DispatchCommand() call to use brace-initialization |
Currently we use ArgSlice, CmdArgList everywhere where we want a span on string_views. As a result, it's very hard to understand the requirements from the passed classes. This PR introduces a new ParsedArguments class and reduces the API surface for the passed object. Make sure to unwind ParsedArguments to ArgSlice only when we start executing the command. We will clean-up redundant aliases in the futures. Signed-off-by: Roman Gershman <[email protected]>
dranikpg
approved these changes
Dec 7, 2025
Comment on lines
+1434
to
+1435
| string cmd = absl::AsciiStrToUpper(args.Front()); | ||
| const auto [cid, args_no_cmd] = registry_.FindExtended(cmd, args.Tail()); |
Contributor
There was a problem hiding this comment.
once we have them packed, we can refactor registry interface to pick front() one by one
vyavdoshenko
approved these changes
Dec 7, 2025
romange
added a commit
that referenced
this pull request
Dec 8, 2025
Following #6171 and #6169 we implement Memcache::Command using BackedArguments. Signed-off-by: Roman Gershman <[email protected]>
romange
added a commit
that referenced
this pull request
Dec 8, 2025
Following #6171 and #6169 we implement Memcache::Command using BackedArguments. Signed-off-by: Roman Gershman <[email protected]>
romange
added a commit
that referenced
this pull request
Dec 8, 2025
Following #6171 and #6169 we implement Memcache::Command using BackedArguments. Signed-off-by: Roman Gershman <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently we use ArgSlice, CmdArgList everywhere where we want a span on string_views. As a result, it's very hard to understand the requirements from the passed classes.
This PR introduces a new ParsedArguments class and reduces the API surface for the passed object. Make sure to unwind ParsedArguments to ArgSlice only when we start executing the command.
We will clean-up redundant aliases in the futures.