Skip to content
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

shell: Refactor command getters #24219

Merged

Conversation

nordic-krch
Copy link
Contributor

Refactor and simplified fetching commands from the tree
of commands.

Main change is changing shell_cmd_get to simply return struct shell_static_entry *. Previously it was getting as argument command level and pointer to command. Those arguments were redundant since command level was only used to distinguish between root command and others. Now, it is simpler as function gets pointer to parent command (null to get one of the root commands). Returns requested entry.

Many algorithms now looks simpler, there is also 250bytes less code (not much but still something).

Note, this refactoring is a start point for further shell improvements around select command.

Signed-off-by: Krzysztof Chruscinski krzysztof.chruscinski@nordicsemi.no

@nordic-krch nordic-krch requested a review from jakub-uC as a code owner April 9, 2020 06:45
@nordic-krch nordic-krch added the area: Shell Shell subsystem label Apr 9, 2020
@@ -252,7 +252,7 @@ static bool tab_prepare(const struct shell *shell,
/* root command completion */
if ((*argc == 0) || ((space == 0) && (*argc == 1))) {
*complete_arg_idx = SHELL_CMD_ROOT_LVL;
*cmd = NULL;
*cmd = shell->ctx->selected_cmd;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

selected_cmd is null when no selection so it can be used to setup parent.

@zephyrbot
Copy link
Collaborator

zephyrbot commented Apr 9, 2020

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.


if (!found) {
if (!is_empty_cmd(candidate) && is_candidate) {
*longest = Z_MAX(strlen(candidate->syntax), *longest);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Z_MAX is "evaluate once" version of MAX

@nordic-krch nordic-krch force-pushed the shell_cmd_get_refactor branch from 722a8b4 to f31209f Compare April 9, 2020 06:49
@jakub-uC
Copy link
Contributor

jakub-uC commented Apr 9, 2020

ojciec_chrzestny

@jakub-uC
Copy link
Contributor

jakub-uC commented Apr 9, 2020

@nordic-krch : Nice simplifaction! :)
Please double-check if wildcards are working correctly and if the Tab button is prompting proposals with wildcards.

@nordic-krch nordic-krch requested a review from nashif as a code owner April 14, 2020 07:12
@zephyrbot zephyrbot added area: API Changes to public APIs area: Tests Issues related to a particular existing or missing test labels Apr 14, 2020
@nordic-krch nordic-krch force-pushed the shell_cmd_get_refactor branch from 084fe15 to f31209f Compare April 14, 2020 07:54
@nordic-krch
Copy link
Contributor Author

beware that more is coming 😄 #24329

@nordic-krch
Copy link
Contributor Author

@jakub-uC ping. I think it is working quite well. I did much more of testing to #24329 which is based on that one.

@carlescufi
Copy link
Member

@nordic-krch could you please rebase?

@nordic-krch nordic-krch force-pushed the shell_cmd_get_refactor branch 2 times, most recently from a067c46 to 2ffab63 Compare April 23, 2020 05:48
Refactor and simplified fetching commands from the tree
of commands.

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
@nordic-krch
Copy link
Contributor Author

@carlescufi done

@nordic-krch
Copy link
Contributor Author

@carlescufi can you merge?

@carlescufi carlescufi merged commit 53b5bae into zephyrproject-rtos:master Apr 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: Shell Shell subsystem area: Tests Issues related to a particular existing or missing test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants