Re: [PATCH 1/3] var_integer -> var_uinteger

dje
2012-08-23T18:20:12Z

Yao Qi writes:
> Hi,
> The ngative value makes no sense to these commands, so this patch simply
> changes them to var_uinteger.

Hi. Most of the patch is ok with me.
Further comments inline.

> gdb:
>
> 2012-08-13 Yao Qi <yao@codesourcery.com>
>
> * cli/cli-cmds.c (max_user_call_depth): Add 'unsigned'.
> (init_cmds): Call add_setshow_uinteger_cmd for command
> 'max-user-call-depth'.
> * cli/cli-script.c (execute_user_command): Add 'unsigned' to the
> declaration of 'max_user_call_depth'.
> * frame.c (backtrace_limit): Add 'unsigned'.
> (_initialize_frame): Call add_setshow_uinteger_cmd for command
> 'limit'.
> * remote.c (remoteaddresssize): Add 'unsigned'.
> (remote_address_masked): Change local var 'address_size' to
> 'unsigned'.
> (_initialize_remote): Call add_setshow_uinteger_cmd for
> 'remoteaddresssize'.
> * top.c (history_size): Add 'unsigned'.
> (show_commands): Change local variables to 'unsigned'.
> (set_history_size_command): Don't check history_size is negative.
> [...]
> diff --git a/gdb/top.c b/gdb/top.c
> index 8251d1b..fbd4120 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -710,7 +710,7 @@ show_write_history_p (struct ui_file *file, int from_tty,
> value);
> }
>
> -static int history_size;
> +static unsigned int history_size;
> static void
> show_history_size (struct ui_file *file, int from_tty,
> struct cmd_list_element *c, const char *value)
> @@ -1379,7 +1379,7 @@ show_commands (char *args, int from_tty)
>
> /* The first command in the history which doesn't exist (i.e. one more
> than the number of the last command). Relative to history_base. */
> - int hist_len;
> + unsigned int hist_len;
>
> /* Print out some of the commands from the command history. */
> /* First determine the length of the history list. */
> @@ -1444,15 +1444,13 @@ show_commands (char *args, int from_tty)
> static void
> set_history_size_command (char *args, int from_tty, struct cmd_list_element *c)
> {
> - if (history_size == INT_MAX)
> + if (history_size == UINT_MAX)
> unstifle_history ();
> - else if (history_size >= 0)
> - stifle_history (history_size);
> + else if (history_size >= INT_MAX)
> + /* The type of parameter in stifle_history is int, so check the range. */
> + error (_("History size must be less than %d"), INT_MAX);

I think we don't want to remove resetting "history_size = INT_MAX;"
in the error case here (but see below).
Also, new style rules say that since the clause is multiple lines
it needs to be wrapped in {}.

It's kinda funny that explicitly setting the value to UINT_MAX (instead
of zero) is ok, but setting the value to anywhere between INT_MAX and
UINT_MAX-1 is an error because it's too big. :-)
That makes me think it'd be better to just bite the bullet and say
any value from INT_MAX and up means "unlimited".
They're effectively that anyway.

> else
> - {
> - history_size = INT_MAX;
> - error (_("History size must be non-negative"));
> - }
> + stifle_history (history_size);
> }
>
> void
> @@ -1632,13 +1630,13 @@ Without an argument, saving is enabled."),
> show_write_history_p,
> &sethistlist, &showhistlist);
>
> - add_setshow_integer_cmd ("size", no_class, &history_size, _("\
> + add_setshow_uinteger_cmd ("size", no_class, &history_size, _("\
> Set the size of the command history,"), _("\
> Show the size of the command history,"), _("\
> ie. the number of previous commands to keep a record of."),
> - set_history_size_command,
> - show_history_size,
> - &sethistlist, &showhistlist);
> + set_history_size_command,
> + show_history_size,
> + &sethistlist, &showhistlist);
>
> add_setshow_filename_cmd ("filename", no_class, &history_filename, _("\
> Set the filename in which to record the command history"), _("\
> --
> 1.7.7.6
>