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.

Sign up for GitHub

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

Jump to bottom

shell: refactor default shell command handling #300

Merged
merged 2 commits into from Dec 3, 2013
Merged

shell: refactor default shell command handling #300

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3be2a32
shell: refactor default shell command handling
kaspar030 Nov 4, 2013
6634c56
shell: fix compile warning and add some comments
kaspar030 Dec 2, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
73 changes: 36 additions & 37 deletions sys/shell/shell.c
View file
Open in desktop
Expand Up @@ -36,58 +36,57 @@

static void(*find_handler(const shell_command_t *command_list, char *command))(char *)
{
const shell_command_t *entry = command_list;

if (entry) {
while (entry->name != NULL) {
if (strcmp(entry->name, command) == 0) {
return entry->handler;
}
else {
entry++;
}
}
}

const shell_command_t *command_lists[] = {
command_list,
#ifdef MODULE_SHELL_COMMANDS
entry = _shell_command_list;

while (entry->name != NULL) {
if (strcmp(entry->name, command) == 0) {
return entry->handler;
}
else {
entry++;
_shell_command_list,
#endif
};

const shell_command_t *entry;

Copy link
Member

Choose a reason for hiding this comment

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

Maybe you could add comments like 'iterate over lists' and 'iterate over list-entries' here and below.
Also, the use of sizeof(entry) irritated me, sizeof(shell_command_t) would make this a little clearer I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added "iterating..." comments.

I've chosen sizeof(entry) instead of sizeof(shell_command_t) because

  1. it's shorter
  2. I can't use typename in sizeof(command_lists) because that's an array and I don't want to use both styles in the same line.
  3. For me "sizeof(lists) / sizeof(entry)" reads clearer than "sizeof(lists) / sizeof(<some_type_name>)"

convince me! ;)

Copy link
Member

Choose a reason for hiding this comment

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

It's not that much of an improvement that I'm going to spend time on trying to argue for it further ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not that much of an improvement that I'm going to spend time on
trying to argue for it further ;-)
Wise man! I choose being stubborn. Feels more visionary. So this is
ACKed from you?

/* iterating over command_lists */
for (unsigned int i = 0; i < sizeof(command_lists)/sizeof(entry); i++) {
if ((entry = command_lists[i])) {
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind adding ' != NULL` here and below to make it clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I'd mind. ;) For me this is perfectly clear.

This is C. if (variable) is true if variable is != NULL or 0. Do we have coding conventions for this?

Copy link
Member

Choose a reason for hiding this comment

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

The idea was to make it obvious that there is a test for NULL, which can be overlooked because one does not expect assignments in if statements. I didn't check, but maybe the coding conventions say something about doing this in the first place.
Second, I personally prefer NULL over 0 in the case of pointers as it is more explicit.

"This is C" does not apply as C does not dictate that code should be hard to read only because it allows for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was to make it obvious that there is a test for NULL, which can be overlooked because one does not expect assignments in if statements. I didn't check, but maybe the coding conventions say something about doing this in the first place.
Second, I personally prefer NULL over 0 in the case of pointers as it is more explicit.

"This is C" does not apply as C does not dictate that code should be hard to read only because it allows for it.
"hard to read" - I think the assignment is easier to read this way.

"This is C" - I like the "shortcut" of not having to use "==0", and it's
the only one of the "bigger" languages that allows this.

Would you need it in, e.g.,
if (is_too_large(packet) == 1)) ...
too to make it "easier to read"?

Copy link
Member

Choose a reason for hiding this comment

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

If you think assignments in comparisons are a good idea then so be it.

/* iterating over commands in command_lists entry */
while (entry->name != NULL) {
if (strcmp(entry->name, command) == 0) {
return entry->handler;
}
else {
entry++;
}
}
}
}

#endif
return NULL;
}

static void print_help(const shell_command_t *command_list)
{
const shell_command_t *entry = command_list;

printf("%-20s %s\n", "Command", "Description");
puts("---------------------------------------");

if (entry) {
while (entry->name != NULL) {
printf("%-20s %s\n", entry->name, entry->desc);
entry++;
}
}

const shell_command_t *command_lists[] = {
command_list,
#ifdef MODULE_SHELL_COMMANDS
entry = _shell_command_list;
_shell_command_list,
#endif
};

while (entry->name != NULL) {
printf("%-20s %s\n", entry->name, entry->desc);
entry++;
}
const shell_command_t *entry;

#endif
/* iterating over command_lists */
for (unsigned int i = 0; i < sizeof(command_lists)/sizeof(entry); i++) {
if ((entry = command_lists[i])) {
/* iterating over commands in command_lists entry */
while (entry->name != NULL) {
printf("%-20s %s\n", entry->name, entry->desc);
entry++;
}
}
}
}

static void handle_input_line(shell_t *shell, char *line)
Expand Down