Re: [RFA] Factor out some functions in remote.c

[Michael Snyder <msnyder@cygnus.com>]

Andrew Cagney wrote:
> 
> > These are two little functions that factor out the conversion between
> > ascii-encoded hex and binary, and vice versa, something that seems to
> > be done inline over and over in remote.c (and something that I was
> > about to do inline yet another time).
> 
> Mostly ok.  Just some tweeks.
> 
> > + /* exported functions */
> > +
> 
> this comment can be deleted.

OK.

> > --- 1731,1738 ----
> >         getpkt (bufp, PBUFSIZ, 0);
> >         if (bufp[0] != 0)
> >       {
> > !       n = min (strlen (bufp) / 2, sizeof (display_buf));
> > !       display_buf [hex2bin (bufp, display_buf, n)] = '\0';
> >         return display_buf;
> >       }
> 
> I needed several takes on this one.  Can you split the line:
> 
>           display_buf [hex2bin (bufp, display_buf, n)] = '\0';
> 
> into two steps?

OK.


> >       }
> > *************** remote_async_detach (char *args, int fro
> > *** 2316,2322 ****
> >
> >   /* Convert hex digit A to a number.  */
> >
> > ! int
> >   fromhex (int a)
> >   {
> >     if (a >= '0' && a <= '9')
> > --- 2309,2315 ----
> >
> >   /* Convert hex digit A to a number.  */
> >
> > ! static int
> >   fromhex (int a)
> >   {
> >     if (a >= '0' && a <= '9')
> > *************** fromhex (int a)
> > *** 2329,2334 ****
> > --- 2322,2350 ----
> >       error ("Reply contains invalid hex digit %d", a);
> >   }
> >
> > + static int
> > + hex2bin (char *hex, char *bin, int count)
> 
> suggest ``const char *hex''.

OK.

> > + {
> > +   int i;
> > +
> > +   /* May use a length, or a nul-terminated string as input. */
> > +   if (count == 0)
> > +     count = strlen (hex) / 2;
> 
> Could I suggest leaving this out.  Looking through the code, this
> doesn't appear to be necessary and it leaves the function open to a
> buffer overrun.

No, I need it.  I just haven't submitted the new code that needs it yet.
I think fixed length buffers and null terminated strings are both reasonable
input.  If you want me to code two separate functions, one for fixed length
buffers and one for null terminated strings, I will (but I'll be sad).

> >     p = buf + strlen (buf);
> >     regp = register_buffer (regno);
> > !   bin2hex (regp, p, REGISTER_RAW_SIZE (regno));
> >     remote_send (buf, PBUFSIZ);
> 
> perhaphs write this as:
> 
>         p += bin2hex (regp, p, ...);
>         *p = '\0';
> 
> so it is clear that the buffer was null terminated.

The output of bin2hex is a string, and it's always null terminated.
I don't think it's reasonable for the caller to have to null terminate
it every time it is called, when the callee can (and should) do it.

I'll submit a new patch based on your feedback.

Michael