Re: [PATCH] Fix uninitialized use of variables.

[Jim Blandy <jimb@codesourcery.com>]

Carlos O'Donell <carlos at codesourcery.com> writes:
> On Tue, Oct 23, 2007 at 04:20:12PM -0700, Jim Blandy wrote:
>> 
>> Carlos O'Donell <carlos at codesourcery.com> writes:
>> > Index: gdb/remote.c
>> > ===================================================================
>> > RCS file: /cvs/src/src/gdb/remote.c,v
>> > retrieving revision 1.271
>> > diff -u -p -r1.271 remote.c
>> > --- gdb/remote.c	8 Oct 2007 12:55:09 -0000	1.271
>> > +++ gdb/remote.c	18 Oct 2007 16:34:05 -0000
>> > @@ -1343,7 +1343,8 @@ unpack_varlen_hex (char *buff,	/* packet
>> >  static char *
>> >  unpack_nibble (char *buf, int *val)
>> >  {
>> > -  ishex (*buf++, val);
>> > +  if (!ishex (*buf++, val))
>> > +    error (_("Unpacked nibble does not contain hex characters."));
>> >    return buf;
>> >  }
>> 
>> This looks fine to me, although Daniel has thoughts on error handling
>> in the remote protocol that I don't fully understand.
>> 
>> But the error message is going to be obscure to users.  It should at
>> least say something about the remote protocol packet being misformed.
>
> In all truth I would have rather returned a status and let the caller
> determine the error message.

I can see the appeal, but that sounds like a lot of work:

- Change all the 'unpack_' functions to take the text pointer by
  reference, update it in place, and return a status.
- Change all their callers to pass the text pointer by reference,
  check the error code, and return an error status.

And the win would be that you can say specifically what part of the
packet didn't parse, instead of just saying that the packet didn't
parse.  It would be cleaner overall, but speaking for myself, there
are other things I'd appreciate more.  :)

The callers of the 'unpack_' functions currently seem to assume that
the callees will raise errors as needed; it doesn't seem too bad to go
along with that.