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

Michael Snyder
2011-02-22T18:25:33Z

[-- 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;
}