Re: [commit] fix for "info threads" printing multiple headers

[Michael Snyder <msnyder@vmware.com>]

[-- Attachment #1: Type: text/plain, Size: 2384 bytes --]

Pedro Alves wrote:
> Thanks!  A few comments below.  If you don't want to
> fix these, I'll try to find a bit later on myself.
> 
> On Monday 21 February 2011 23:40:06, Michael Snyder wrote:
>> +int
>> +number_is_in_list (char *list, int number)
>> +{
>> +  if (list == NULL || *list == '\0')
>> +    return 1;
>> +
>> +  while (list != NULL && *list != '\0')
>> +    if (get_number_or_range (&list) == number)
>> +      return 1;
>> +
>> +  return 0;
>> +}
> 
> - why the 'list != NULL' check in the while loop?
> get_number_or_range never puts NULL in the output
> argument, and NULL lists are short circuited above.

Habit and symmetry.  I'll change it.

> 
> - get_number_or_range mantains an internal state machine
> using static variables.  I think that as long as you
> always pass in the same list string, and the list spec
> string is correctly formed, you're not hitting stale
> state inside get_number_or_range.  It'd be nicer
> if get_number_or_range (or a variant which get_number_or_range
> would then be implemented on top of) took an additional
> struct pointer that pointed to a struct that held
> all the currently static state.  This function would then
> always pass in a pointer into such a newly initialized
> object on the stack.
> 
> - get_number_or_range returns '0' on error.  You can
> see a bunch of its old callers in breakpoint.c handling
> that '0' case specially.  It comes from get_number_trailer:
> 
> 	  /* Return zero, which caller must interpret as error.  */
> 	  retval = 0;
> 
> I suspect that we'll want to make than an "error" call or
> to adjust the interface of these functions to allow
> returning an error indication unambiguous with zero
> the number, as soon as we want to reuse this function
> in places where zero makes sense as valid number...
> 
> Examples of not handling the special zero:
> 
> (gdb) info threads a 1
>   Id   Target Id         Frame 
> * 1    Thread 0x7ffff7fd6700 (LWP 13769) "threads" 0x00007ffff7bc7285 in pthread_join () from /lib/libpthread.so.0
> (gdb)
> 
> (gdb) info threads a-1
> No threads match 'a-1'.
> 
> (gdb) info threads a -1
>   Id   Target Id         Frame 
> * 1    Thread 0x7ffff7fd6700 (LWP 13769) "threads" 0x00007ffff7bc7285 in pthread_join () from /lib/libpthread.so.0
> (gdb) 
> 
> The last one only "works" by accident due to the extra space.


OK, test for zero added, how's this?






[-- Attachment #2: zero.txt --]
[-- Type: text/plain, Size: 859 bytes --]

2011-02-22  Michael Snyder  <msnyder@vmware.com>

	* cli/cli-utils.c (number_is_in_list): Check for zero return.

Index: cli/cli-utils.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-utils.c,v
retrieving revision 1.9
diff -u -p -u -p -r1.9 cli-utils.c
--- cli/cli-utils.c	21 Feb 2011 23:40:46 -0000	1.9
+++ cli/cli-utils.c	22 Feb 2011 18:24:22 -0000
@@ -175,10 +175,15 @@ number_is_in_list (char *list, int numbe
   if (list == NULL || *list == '\0')
     return 1;
 
-  while (list != NULL && *list != '\0')
-    if (get_number_or_range (&list) == number)
-      return 1;
+  while (*list != '\0')
+    {
+      int gotnum = get_number_or_range (&list);
 
+      if (gotnum == 0)
+	error (_("Args must be numbers or '$' variables."));
+      if (gotnum == number)
+	return 1;
+    }
   return 0;
 }