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?