[PATCH 0/4] Tweak get_number related code

[PATCH 0/4] Tweak get_number related code

[Yao Qi]

Hi,
When I work on a patch using get_number in MI, I realize that code on
using get_number should be cleaned up first.  This patch series is
part of this cleanup, and I think they can be submitted first.  Each
patch in this series is quite independent.  Changes are obvious but
review is welcome too.

Regression tested on x86_64-linux.

*** BLURB HERE ***

Yao Qi (4):
  Add a newline in output messages
  Error on bad count number
  Handle parse number error in goto_bookmark_command
  Remove argument optional_p from get_tracepoint_by_number

 gdb/breakpoint.c                      |   26 ++++++++++++--------------
 gdb/breakpoint.h                      |    3 +--
 gdb/cli/cli-utils.c                   |    2 +-
 gdb/reverse.c                         |    7 ++++++-
 gdb/testsuite/gdb.base/ena-dis-br.exp |    5 +++++
 gdb/tracepoint.c                      |    2 +-
 6 files changed, 26 insertions(+), 19 deletions(-)

-- 
1.7.7.6

[PATCH 3/4] Handle parse number error in goto_bookmark_command

[Yao Qi]

In GDB mainline, the error message for goto-bookmark
isn't perfect.

 (gdb) goto-bookmark 1.1
 goto-bookmark: no bookmark found for ''.

This patch tweaks the error message by checking the return value of
get_number.  With patch applied, it becomes:

 (gdb) goto-bookmark 1.1
 goto-bookmark: no bookmark found for '1.1'.

gdb:

2014-03-05  Yao Qi  <yao@codesourcery.com>

	* reverse.c (goto_bookmark_command): Add local 'p'.  Emit error
	early if get_number returns zero.  Use 'p' instead of 'args'.
---
 gdb/reverse.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/gdb/reverse.c b/gdb/reverse.c
index 582252c..0ceecf9 100644
--- a/gdb/reverse.c
+++ b/gdb/reverse.c
@@ -250,6 +250,7 @@ goto_bookmark_command (char *args, int from_tty)
 {
   struct bookmark *b;
   unsigned long num;
+  char *p = args;
 
   if (args == NULL || args[0] == '\0')
     error (_("Command requires an argument."));
@@ -274,6 +275,10 @@ goto_bookmark_command (char *args, int from_tty)
 
   /* General case.  Bookmark identified by bookmark number.  */
   num = get_number (&args);
+
+  if (num == 0)
+    error (_("goto-bookmark: no bookmark found for '%s'."), p);
+
   ALL_BOOKMARKS (b)
     if (b->number == num)
       break;
@@ -285,7 +290,7 @@ goto_bookmark_command (char *args, int from_tty)
       return;
     }
   /* Not found.  */
-  error (_("goto-bookmark: no bookmark found for '%s'."), args);
+  error (_("goto-bookmark: no bookmark found for '%s'."), p);
 }
 
 static int
-- 
1.7.7.6

Re: [PATCH 3/4] Handle parse number error in goto_bookmark_command

[Joel Brobecker]

>  (gdb) goto-bookmark 1.1
>  goto-bookmark: no bookmark found for '1.1'.

I would use "invalid bookmark number"...

> 2014-03-05  Yao Qi  <yao@codesourcery.com>
> 
> 	* reverse.c (goto_bookmark_command): Add local 'p'.  Emit error
> 	early if get_number returns zero.  Use 'p' instead of 'args'.

Otherwise, LGTM.


> ---
>  gdb/reverse.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/gdb/reverse.c b/gdb/reverse.c
> index 582252c..0ceecf9 100644
> --- a/gdb/reverse.c
> +++ b/gdb/reverse.c
> @@ -250,6 +250,7 @@ goto_bookmark_command (char *args, int from_tty)
>  {
>    struct bookmark *b;
>    unsigned long num;
> +  char *p = args;
>  
>    if (args == NULL || args[0] == '\0')
>      error (_("Command requires an argument."));
> @@ -274,6 +275,10 @@ goto_bookmark_command (char *args, int from_tty)
>  
>    /* General case.  Bookmark identified by bookmark number.  */
>    num = get_number (&args);
> +
> +  if (num == 0)
> +    error (_("goto-bookmark: no bookmark found for '%s'."), p);
> +
>    ALL_BOOKMARKS (b)
>      if (b->number == num)
>        break;
> @@ -285,7 +290,7 @@ goto_bookmark_command (char *args, int from_tty)
>        return;
>      }
>    /* Not found.  */
> -  error (_("goto-bookmark: no bookmark found for '%s'."), args);
> +  error (_("goto-bookmark: no bookmark found for '%s'."), p);
>  }
>  
>  static int
> -- 
> 1.7.7.6

-- 
Joel

Re: [PATCH 3/4] Handle parse number error in goto_bookmark_command

[Yao Qi]

On 03/05/2014 10:38 PM, Joel Brobecker wrote:
>>  (gdb) goto-bookmark 1.1
>> >  goto-bookmark: no bookmark found for '1.1'.
> I would use "invalid bookmark number"...
> 

OK, this message is fine to me.  Patch is updated and pushed in.

-- 
Yao (齐尧)

    Handle parse number error in goto_bookmark_command
    
    In GDB mainline, the error message for goto-bookmark
    isn't perfect.
    
     (gdb) goto-bookmark 1.1
     goto-bookmark: no bookmark found for ''.
    
    This patch tweaks the error message by checking the return value of
    get_number.  With patch applied, it becomes:
    
     (gdb) goto-bookmark 1.1
     goto-bookmark: invalid bookmark number '1.1'.
    
    gdb:
    
    2014-03-06  Yao Qi  <yao@codesourcery.com>
    
    	* reverse.c (goto_bookmark_command): Add local 'p'.  Emit error
    	early if get_number returns zero.  Use 'p' instead of 'args'.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 8088e64..3a1124a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,10 @@
 2014-03-06  Yao Qi  <yao@codesourcery.com>
 
+	* reverse.c (goto_bookmark_command): Add local 'p'.  Emit error
+	early if get_number returns zero.  Use 'p' instead of 'args'.
+
+2014-03-06  Yao Qi  <yao@codesourcery.com>
+
 	* cli/cli-utils.c (get_number_trailer): Add '\n' at the end of
 	message.
 
diff --git a/gdb/reverse.c b/gdb/reverse.c
index 582252c..30a0328 100644
--- a/gdb/reverse.c
+++ b/gdb/reverse.c
@@ -250,6 +250,7 @@ goto_bookmark_command (char *args, int from_tty)
 {
   struct bookmark *b;
   unsigned long num;
+  char *p = args;
 
   if (args == NULL || args[0] == '\0')
     error (_("Command requires an argument."));
@@ -274,6 +275,10 @@ goto_bookmark_command (char *args, int from_tty)
 
   /* General case.  Bookmark identified by bookmark number.  */
   num = get_number (&args);
+
+  if (num == 0)
+    error (_("goto-bookmark: invalid bookmark number '%s'."), p);
+
   ALL_BOOKMARKS (b)
     if (b->number == num)
       break;
@@ -285,7 +290,7 @@ goto_bookmark_command (char *args, int from_tty)
       return;
     }
   /* Not found.  */
-  error (_("goto-bookmark: no bookmark found for '%s'."), args);
+  error (_("goto-bookmark: no bookmark found for '%s'."), p);
 }
 
 static int

[PATCH 4/4] Remove argument optional_p from get_tracepoint_by_number

[Yao Qi]

This patch is to remove parameter optional_p as it is always true,
in order to simplify get_tracepoint_by_number.

'optional_p' was added by this change,

1999-11-18  Tom Tromey  <tromey@cygnus.com>

	* tracepoint.h (get_tracepoint_by_number): Updated
	declaration.
	* tracepoint.c (trace_pass_command): Better error message.
	Fixed logic when `all' not specified.
	(get_tracepoint_by_number): Added `optional_p' argument.  Fixed
	all callers.

but after this patch,

 FYI: remove `static's from cli-utils.c
 https://sourceware.org/ml/gdb-patches/2011-03/msg00636.html

'optional_p' passed to get_tracepoint_by_number become always true.

gdb:

2014-03-05  Yao Qi  <yao@codesourcery.com>

	* breakpoint.c (get_tracepoint_by_number): Remove argument
	optional_p.  All callers updated.  Adjust comments.  Update
	output message.
	* breakpoint.h (get_tracepoint_by_number): Update declaration.
---
 gdb/breakpoint.c |   22 ++++++++--------------
 gdb/breakpoint.h |    3 +--
 gdb/tracepoint.c |    2 +-
 3 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 5dcaa0e..5b5dd10 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -15536,7 +15536,7 @@ trace_pass_command (char *args, int from_tty)
     }
   else if (*args == '\0')
     {
-      t1 = get_tracepoint_by_number (&args, NULL, 1);
+      t1 = get_tracepoint_by_number (&args, NULL);
       if (t1)
 	trace_pass_set_count (t1, count, from_tty);
     }
@@ -15547,7 +15547,7 @@ trace_pass_command (char *args, int from_tty)
       init_number_or_range (&state, args);
       while (!state.finished)
 	{
-	  t1 = get_tracepoint_by_number (&args, &state, 1);
+	  t1 = get_tracepoint_by_number (&args, &state);
 	  if (t1)
 	    trace_pass_set_count (t1, count, from_tty);
 	}
@@ -15588,12 +15588,12 @@ get_tracepoint_by_number_on_target (int num)
 
 /* Utility: parse a tracepoint number and look it up in the list.
    If STATE is not NULL, use, get_number_or_range_state and ignore ARG.
-   If OPTIONAL_P is true, then if the argument is missing, the most
-   recent tracepoint (tracepoint_count) is returned.  */
+   If the argument is missing, the most recent tracepoint
+   (tracepoint_count) is returned.  */
+
 struct tracepoint *
 get_tracepoint_by_number (char **arg,
-			  struct get_number_or_range_state *state,
-			  int optional_p)
+			  struct get_number_or_range_state *state)
 {
   struct breakpoint *t;
   int tpnum;
@@ -15605,12 +15605,7 @@ get_tracepoint_by_number (char **arg,
       tpnum = get_number_or_range (state);
     }
   else if (arg == NULL || *arg == NULL || ! **arg)
-    {
-      if (optional_p)
-	tpnum = tracepoint_count;
-      else
-	error_no_arg (_("tracepoint number"));
-    }
+    tpnum = tracepoint_count;
   else
     tpnum = get_number (arg);
 
@@ -15620,8 +15615,7 @@ get_tracepoint_by_number (char **arg,
 	printf_filtered (_("bad tracepoint number at or near '%s'\n"), 
 			 instring);
       else
-	printf_filtered (_("Tracepoint argument missing "
-			   "and no previous tracepoint\n"));
+	printf_filtered (_("No previous tracepoint\n"));
       return NULL;
     }
 
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 3c1fdfe..bf1f52a 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1491,8 +1491,7 @@ extern struct tracepoint *get_tracepoint_by_number_on_target (int num);
 /* Find a tracepoint by parsing a number in the supplied string.  */
 extern struct tracepoint *
      get_tracepoint_by_number (char **arg, 
-			       struct get_number_or_range_state *state,
-			       int optional_p);
+			       struct get_number_or_range_state *state);
 
 /* Return a vector of all tracepoints currently defined.  The vector
    is newly allocated; the caller should free when done with it.  */
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 1ff1bda..e1e2fce 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -653,7 +653,7 @@ trace_actions_command (char *args, int from_tty)
   struct tracepoint *t;
   struct command_line *l;
 
-  t = get_tracepoint_by_number (&args, NULL, 1);
+  t = get_tracepoint_by_number (&args, NULL);
   if (t)
     {
       char *tmpbuf =
-- 
1.7.7.6

Re: [PATCH 4/4] Remove argument optional_p from get_tracepoint_by_number

[Joel Brobecker]

> This patch is to remove parameter optional_p as it is always true,
> in order to simplify get_tracepoint_by_number.
> 
> 'optional_p' was added by this change,
> 
> 1999-11-18  Tom Tromey  <tromey@cygnus.com>
> 
> 	* tracepoint.h (get_tracepoint_by_number): Updated
> 	declaration.
> 	* tracepoint.c (trace_pass_command): Better error message.
> 	Fixed logic when `all' not specified.
> 	(get_tracepoint_by_number): Added `optional_p' argument.  Fixed
> 	all callers.
> 
> but after this patch,
> 
>  FYI: remove `static's from cli-utils.c
>  https://sourceware.org/ml/gdb-patches/2011-03/msg00636.html
> 
> 'optional_p' passed to get_tracepoint_by_number become always true.

FWIW - don't know much about this code, but this patch looks fine to me.

-- 
Joel

[OBV PATCH 1/4] Add a newline in output messages

[Yao Qi]

Hi,
GDB prints two warnings in one single line, as below:

 (gdb) p 1.2
 $1 = 1.2
 (gdb) enable $1.2
 History value must have integer type.Bad breakpoint number '$1'

This patch adds '\n' at the end of message.

gdb:

2014-03-05  Yao Qi  <yao@codesourcery.com>

	* cli/cli-utils.c (get_number_trailer): Add '\n' at the end of
	message.
---
 gdb/cli/cli-utils.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gdb/cli/cli-utils.c b/gdb/cli/cli-utils.c
index e18e357..a0ebc11 100644
--- a/gdb/cli/cli-utils.c
+++ b/gdb/cli/cli-utils.c
@@ -50,7 +50,7 @@ get_number_trailer (char **pp, int trailer)
 	    retval = value_as_long (val);
 	  else
 	    {
-	      printf_filtered (_("History value must have integer type."));
+	      printf_filtered (_("History value must have integer type.\n"));
 	      retval = 0;
 	    }
 	}
-- 
1.7.7.6

Re: [OBV PATCH 1/4] Add a newline in output messages

[Joel Brobecker]

> 2014-03-05  Yao Qi  <yao@codesourcery.com>
> 
> 	* cli/cli-utils.c (get_number_trailer): Add '\n' at the end of
> 	message.

Obvious indeed.

The error handling is a little different from the usual, where error
would typically be used; especially since zero can be a valid number.
And this leads to this sort of odd behavior:

    (gdb) set $v := 1.1
    (gdb) b a
    Breakpoint 1 at 0x401558: file a.adb, line 3.
    (gdb) cond $v 1 = 1
    Convenience variable must have integer value.
    Bad breakpoint argument: '$v 1 = 1'

Probably for a rainy day...

> ---
>  gdb/cli/cli-utils.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/gdb/cli/cli-utils.c b/gdb/cli/cli-utils.c
> index e18e357..a0ebc11 100644
> --- a/gdb/cli/cli-utils.c
> +++ b/gdb/cli/cli-utils.c
> @@ -50,7 +50,7 @@ get_number_trailer (char **pp, int trailer)
>  	    retval = value_as_long (val);
>  	  else
>  	    {
> -	      printf_filtered (_("History value must have integer type."));
> +	      printf_filtered (_("History value must have integer type.\n"));
>  	      retval = 0;
>  	    }
>  	}
> -- 
> 1.7.7.6

-- 
Joel

Re: [OBV PATCH 1/4] Add a newline in output messages

[Yao Qi]

On 03/05/2014 10:22 PM, Joel Brobecker wrote:
> Obvious indeed.
> 

Patch is pushed in.

> The error handling is a little different from the usual, where error
> would typically be used; especially since zero can be a valid number.

Yes, a message is printed out, and let the caller to decide whether to
throw an error.

> And this leads to this sort of odd behavior:
> 
>     (gdb) set $v := 1.1
>     (gdb) b a
>     Breakpoint 1 at 0x401558: file a.adb, line 3.
>     (gdb) cond $v 1 = 1
>     Convenience variable must have integer value.
>     Bad breakpoint argument: '$v 1 = 1'

Sorry, I don't find anything odd here.  Is the odd behavior to print two
lines for an invalid input?

-- 
Yao (齐尧)

[PATCH 2/4] Error on bad count number

[Yao Qi]

GDB is quiet for these invalid input like this

  (gdb) enable count 1.1 1
  (gdb)

This patch is to check the input number is valid, and emit error if
input number isn't valid.  With this patch, it becomes:

  (gdb) enable count 1.1 1
  Bad count number '1.1 1'

gdb:

2014-03-05  Yao Qi  <yao@codesourcery.com>

	* breakpoint.c (enable_count_command): Emit error if 'count'
	is zero.

gdb/testsuite:

2014-03-05  Yao Qi  <yao@codesourcery.com>

	* gdb.base/ena-dis-br.exp: Test bad count number.
---
 gdb/breakpoint.c                      |    4 ++++
 gdb/testsuite/gdb.base/ena-dis-br.exp |    5 +++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 2f2c625..5dcaa0e 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -14934,8 +14934,12 @@ do_map_enable_count_breakpoint (struct breakpoint *bpt, void *countptr)
 static void
 enable_count_command (char *args, int from_tty)
 {
+  char *p = args;
   int count = get_number (&args);
 
+  if (count == 0)
+    error (_("Bad count number '%s'"), p);
+
   map_breakpoint_numbers (args, do_map_enable_count_breakpoint, &count);
 }
 
diff --git a/gdb/testsuite/gdb.base/ena-dis-br.exp b/gdb/testsuite/gdb.base/ena-dis-br.exp
index 6f2c469..4b83e56 100644
--- a/gdb/testsuite/gdb.base/ena-dis-br.exp
+++ b/gdb/testsuite/gdb.base/ena-dis-br.exp
@@ -169,6 +169,11 @@ gdb_test "continue" \
     ".*marker1 .*:($bp_location15|$bp_location16).*" \
     "continue through enable count, now disabled"
 
+# Test enable count with bad count number.
+
+gdb_test "enable count 1.1 $bp" "Bad count number '1.1 $bp'.*" \
+    "enable count with bad number"
+
 # Verify that we can set a breakpoint with an ignore count N, which
 # should cause the next N triggers of the bp to be ignored.  (This is
 # a flavor of enablement/disablement, after all.)
-- 
1.7.7.6

Re: [PATCH 2/4] Error on bad count number

[Joel Brobecker]

> GDB is quiet for these invalid input like this
> 
>   (gdb) enable count 1.1 1
>   (gdb)
> 
> This patch is to check the input number is valid, and emit error if
> input number isn't valid.  With this patch, it becomes:
> 
>   (gdb) enable count 1.1 1
>   Bad count number '1.1 1'
> 
> gdb:
> 
> 2014-03-05  Yao Qi  <yao@codesourcery.com>
> 
> 	* breakpoint.c (enable_count_command): Emit error if 'count'
> 	is zero.
> 
> gdb/testsuite:
> 
> 2014-03-05  Yao Qi  <yao@codesourcery.com>
> 
> 	* gdb.base/ena-dis-br.exp: Test bad count number.

I had a slight hesitation here, as we are now going to actively
reject "enable count 0 1" where it used to be accepted. But since
it makes little sense, and does not work, I think that's OK.
Example of it not working:

    (gdb) b a
    Breakpoint 1 at 0x401558: file a.adb, line 3.
    (gdb) enable count 0 1
    (gdb) run
    Starting program: /[...]/a
    Breakpoint 1, a () at a.adb:3
    3          raise Constraint_Error;

The good news is that the breakpoint gets disabled afterwards,
so it's not completely broken:

    (gdb) info break
    Num     Type           Disp Enb Address            What
    1       breakpoint     dis  n   0x0000000000401558 in a at a.adb:3
            breakpoint already hit 1 time

> +  if (count == 0)
> +    error (_("Bad count number '%s'"), p);

I would change it to "Invalid count number" instead.

> +
>    map_breakpoint_numbers (args, do_map_enable_count_breakpoint, &count);
>  }
>  
> diff --git a/gdb/testsuite/gdb.base/ena-dis-br.exp b/gdb/testsuite/gdb.base/ena-dis-br.exp
> index 6f2c469..4b83e56 100644
> --- a/gdb/testsuite/gdb.base/ena-dis-br.exp
> +++ b/gdb/testsuite/gdb.base/ena-dis-br.exp
> @@ -169,6 +169,11 @@ gdb_test "continue" \
>      ".*marker1 .*:($bp_location15|$bp_location16).*" \
>      "continue through enable count, now disabled"
>  
> +# Test enable count with bad count number.
> +
> +gdb_test "enable count 1.1 $bp" "Bad count number '1.1 $bp'.*" \
> +    "enable count with bad number"

Can you also add a test for '0' as the count number?

>  # Verify that we can set a breakpoint with an ignore count N, which
>  # should cause the next N triggers of the bp to be ignored.  (This is
>  # a flavor of enablement/disablement, after all.)

Thank you,
-- 
Joel

Re: [PATCH 2/4] Error on bad count number

[Pedro Alves]

On 03/05/2014 02:29 PM, Joel Brobecker wrote:
>> GDB is quiet for these invalid input like this
>>
>>   (gdb) enable count 1.1 1
>>   (gdb)
>>
>> This patch is to check the input number is valid, and emit error if
>> input number isn't valid.  With this patch, it becomes:
>>
>>   (gdb) enable count 1.1 1
>>   Bad count number '1.1 1'
>>
>> gdb:
>>
>> 2014-03-05  Yao Qi  <yao@codesourcery.com>
>>
>> 	* breakpoint.c (enable_count_command): Emit error if 'count'
>> 	is zero.
>>
>> gdb/testsuite:
>>
>> 2014-03-05  Yao Qi  <yao@codesourcery.com>
>>
>> 	* gdb.base/ena-dis-br.exp: Test bad count number.
> 
> I had a slight hesitation here, as we are now going to actively
> reject "enable count 0 1" where it used to be accepted. But since
> it makes little sense, and does not work, I think that's OK.
> Example of it not working:

I had the same thought.  I wondered whether 0 was meant to
remove the enable count, but the docs don't say anything
about it.  Is there another way to disable the count, and
bring back the disposition to enabled?  If not, I wonder
whether using 0 for that would be a good idea?

-- 
Pedro Alves

Re: [PATCH 2/4] Error on bad count number

[Yao Qi]

On 03/05/2014 11:58 PM, Pedro Alves wrote:
> I had the same thought.  I wondered whether 0 was meant to
> remove the enable count, but the docs don't say anything
> about it.  Is there another way to disable the count, and
> bring back the disposition to enabled?  If not, I wonder
> whether using 0 for that would be a good idea?

If 0 is potentially acceptable, I am fine to keep it.  I'll back
to change get_number to return negative for invalid input, to
differentiate valid number zero and invalid input.

-- 
Yao (齐尧)

Re: [PATCH 2/4] Error on bad count number

[Pedro Alves]

On 03/06/2014 09:52 AM, Yao Qi wrote:
> On 03/05/2014 11:58 PM, Pedro Alves wrote:
>> I had the same thought.  I wondered whether 0 was meant to
>> remove the enable count, but the docs don't say anything
>> about it.  Is there another way to disable the count, and
>> bring back the disposition to enabled?  If not, I wonder
>> whether using 0 for that would be a good idea?
> 
> If 0 is potentially acceptable, I am fine to keep it.  I'll back
> to change get_number to return negative for invalid input, to
> differentiate valid number zero and invalid input.

AFAICS, get_number handles negative numbers.  E.g.,

 (top-gdb) ignore -6 1
 Will ignore next crossing of breakpoint -6.

I think that what exact number is used for error return
is mostly irrelevant.  The interface of get_number's value
return alone is just not sufficient as is.  Not changing
the prototype, we could e.g. in addition of checking for
zero return, have the callers check whether the passed in
char pointer pointer advanced (and make sure get_number
doesn't advance the pointer on invalid input).  So the
correct use becomes:

  p = arg;
  num = get_number (&p);
  if (num == 0 && p == arg)
    error (_("Bad number: '%s'"), arg);

Sort of like strtol vs itoa.

-- 
Pedro Alves

Re: [PATCH 2/4] Error on bad count number

[Joel Brobecker]

> I think that what exact number is used for error return
> is mostly irrelevant.  The interface of get_number's value
> return alone is just not sufficient as is.  Not changing
> the prototype, we could e.g. in addition of checking for
> zero return, have the callers check whether the passed in
> char pointer pointer advanced (and make sure get_number
> doesn't advance the pointer on invalid input).  So the
> correct use becomes:
> 
>   p = arg;
>   num = get_number (&p);
>   if (num == 0 && p == arg)
>     error (_("Bad number: '%s'"), arg);
> 
> Sort of like strtol vs itoa.

There are not so many calls to that routine or (get_number_trailer),
so I think it would be easy to change the interface. But better yet,
why not throw an error? Most of the current uses just throw an error
right after when the returned value was 0. There are some uses where
there is no error handling, but I think they need to be reviewed as
they could actually benefit from it. After a quick glance, it wasn't
obvious to me whether there was a hole in the implementation or not.

-- 
Joel

Re: [PATCH 2/4] Error on bad count number

[Yao Qi]

On 03/06/2014 08:21 PM, Pedro Alves wrote:
> AFAICS, get_number handles negative numbers.  E.g.,
> 

Yes, get_number handles negative, but is it expected for get_number
to handle negative?

>  (top-gdb) ignore -6 1
>  Will ignore next crossing of breakpoint -6.

Shouldn't GDB emit an error on the negative input?  Do we have any
case that negative is valid?  Note that get_number_or_range errors on
negative.

  (gdb) thread apply -2-1 bt
  negative value

-- 
Yao (齐尧)

Re: [PATCH 2/4] Error on bad count number

[Eli Zaretskii]

> Date: Fri, 7 Mar 2014 17:38:19 +0800
> From: Yao Qi <yao@codesourcery.com>
> CC: Joel Brobecker <brobecker@adacore.com>, <gdb-patches@sourceware.org>
> 
> >  (top-gdb) ignore -6 1
> >  Will ignore next crossing of breakpoint -6.
> 
> Shouldn't GDB emit an error on the negative input?  Do we have any
> case that negative is valid?

AFAIR, breakpoints set by GDB for its own purposes are given negative
numbers.

Re: [PATCH 2/4] Error on bad count number

[Yao Qi]

On 03/07/2014 06:53 PM, Eli Zaretskii wrote:
> AFAIR, breakpoints set by GDB for its own purposes are given negative
> numbers.

Yes, the number of breakpoint for specific event, such as shlib event
and thread event, is negative.  These negative numbers are used by GDB
internally, negative number in user input is still invalid, afaik.

-- 
Yao (齐尧)

Re: [PATCH 2/4] Error on bad count number

[Pedro Alves]

On 03/07/2014 09:38 AM, Yao Qi wrote:
> On 03/06/2014 08:21 PM, Pedro Alves wrote:
>> > AFAICS, get_number handles negative numbers.  E.g.,
>> > 
> Yes, get_number handles negative, but is it expected for get_number
> to handle negative?

Nothing in a name like "get number" suggests to me only positive
numbers would be returned.  I think it should be up to the
caller to handle whether the returned value is valid in
that context.

-- 
Pedro Alves

Re: [PATCH 2/4] Error on bad count number

[Joel Brobecker]

> Nothing in a name like "get number" suggests to me only positive
> numbers would be returned.  I think it should be up to the
> caller to handle whether the returned value is valid in
> that context.

I agree. If useful, we could have a second routine that calls
get_number and errors out if the number returned was negative,
like get_natural_number for instance.

-- 
Joel

Re: [PATCH 2/4] Error on bad count number

[Yao Qi]

On 03/07/2014 09:27 PM, Pedro Alves wrote:
> On 03/07/2014 09:38 AM, Yao Qi wrote:
>> On 03/06/2014 08:21 PM, Pedro Alves wrote:
>>>> AFAICS, get_number handles negative numbers.  E.g.,
>>>>
>> Yes, get_number handles negative, but is it expected for get_number
>> to handle negative?
> 
> Nothing in a name like "get number" suggests to me only positive
> numbers would be returned.  I think it should be up to the

You didn't address the second half of my comments that
get_number_or_range errors on negative number.

> caller to handle whether the returned value is valid in
> that context.
> 

This is sort of what I want to do.  I'd like to restrict get_number
and get_number_or_range to only handle non-negative number, and return
negative number as error code.  The callers of get_number or
get_number_or_range can decide to emit warning or error in their own
context.  Checking how char pointer pointer advanced can only tell a
boolean state about error is encountered or not, but no details of
the type of the error.

I want to use get_number in mi_cmd_break_commands, and returns error
code about "not-a-number" or "junk at the end" respectively.

-- 
Yao (齐尧)

Re: [PATCH 2/4] Error on bad count number

[Joel Brobecker]

> I had the same thought.  I wondered whether 0 was meant to
> remove the enable count, but the docs don't say anything
> about it.  Is there another way to disable the count, and
> bring back the disposition to enabled?  If not, I wonder
> whether using 0 for that would be a good idea?

I forgot to answer this. I did not realize that one could not
remove the counter. zero would work. Perhaps we could also
have a special keyword that would make things a little more
explicit?

-- 
Joel