Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: dje@google.com
To: Yao Qi <yao@codesourcery.com>
Cc: <gdb-patches@sourceware.org>
Subject: Re: [PATCH 1/3] var_integer -> var_uinteger
Date: Thu, 23 Aug 2012 18:20:00 -0000	[thread overview]
Message-ID: <20534.29766.629459.204573@ruffy2.mtv.corp.google.com> (raw)
In-Reply-To: <1344871663-915-2-git-send-email-yao@codesourcery.com>

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
 > 


  reply	other threads:[~2012-08-23 18:20 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-07-18 12:52 [PATCH] Handle var_uinteger/var_zuinteger and case var_integer/var_zinteger together Yao Qi
2012-07-24 12:51 ` [committed]: " Yao Qi
2012-07-27 17:40 ` Khoo Yit Phang
2012-07-29 14:25   ` Yao Qi
2012-08-01 13:56   ` [RFC 0/3] New var_types var_zuinteger_unlimited Yao Qi
2012-08-01 13:56     ` [PATCH 1/3] var_zuinteger_unlimited and 'set listsize' Yao Qi
2012-08-01 16:14       ` Eli Zaretskii
2012-08-02  8:34       ` Doug Evans
2012-08-02 12:53         ` Yao Qi
2012-08-01 13:56     ` [PATCH 3/3] use zuinteger_unlimited for heuristic-fence-post Yao Qi
2012-08-01 13:56     ` [PATCH 2/3] use zuinteger_unlimited for some remote commands Yao Qi
2012-08-13 15:28   ` [RFC 0/3] Get rid of var_integer in CLI Yao Qi
2012-08-13 15:28     ` [PATCH 3/3] comments update Yao Qi
2012-09-14 18:13       ` Tom Tromey
2012-08-13 15:28     ` [PATCH 2/3] var_integer -> var_zuinteger_unlimited Yao Qi
2012-08-13 17:54       ` Eli Zaretskii
2012-09-14 18:12       ` Tom Tromey
2012-09-17  8:43         ` [committed]: " Yao Qi
2012-08-13 15:28     ` [PATCH 1/3] var_integer -> var_uinteger Yao Qi
2012-08-23 18:20       ` dje [this message]
2012-08-24  6:56         ` Yao Qi
2012-08-24 17:06           ` dje
2012-08-27 10:10             ` Yao Qi
2012-08-27 22:14               ` dje
2012-08-28 14:09                 ` [committed]: " Yao Qi
2013-03-25 22:55                   ` Change "set history size" back to signed (Re: [committed]: [PATCH 1/3] var_integer -> var_uinteger) Pedro Alves
2013-03-26 16:48                     ` Yao Qi
2013-03-26 17:48                       ` Pedro Alves
2013-03-27  8:54                         ` Yao Qi
2013-03-27 17:34                           ` Pedro Alves
2012-08-22 14:30     ` [ping] : [RFC 0/3] Get rid of var_integer in CLI Yao Qi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20534.29766.629459.204573@ruffy2.mtv.corp.google.com \
    --to=dje@google.com \
    --cc=gdb-patches@sourceware.org \
    --cc=yao@codesourcery.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox