Thanks for the review, David. Agreed on all points. J. On Thu, 05 Apr 2012 08:56:24 -0300, David Bremner <bremner@unb.ca> wrote: > Jani Nikula <jani@nikula.org> writes: > > > Add function hex_decode_inplace() to decode the input string onto > > itself. > > > > Signed-off-by: Jani Nikula <jani@nikula.org> > > > > --- > > > > This could be folded to "hex-escape: (en|de)code strings to/from > > restricted character set". > > Probably. It's a bit hard to follow as is; somehow the code movement is > a bit noisy. > > > while (*p) { > > - > > if (*p == escape_char) { > > - > unrelated whitespace changes, naughty naughty. > > > +hex_status_t > > +hex_decode_inplace (char *p) > > +{ > > + return hex_decode_internal (p, (unsigned char *) p); > this could probably use a comment to the effect that there _will_ be > enough space. > > > + > > + p = in; > > + q = (unsigned char *) *out; > > I understand trying to minimize changes, but do p and q serve any > purpose here? > > > + > > + return hex_decode_internal (p, q); > > +} > > > > +/* > > + * Decode 'in' onto itself. > > + */ > > This is just a bit terse for my taste. > > d