From: Jani Nikula <jani@nikula.org>
To: David Bremner <bremner@unb.ca>, notmuch@notmuchmail.org
Subject: Re: [PATCH 3/8] hex-escape: add function to decode escaped string
	in-place
In-Reply-To: <87d37mtms7.fsf@zancas.localnet>
References: <cover.1333231401.git.jani@nikula.org>
	<026815054d3e01e6f29c0834d43ccabdf6eda481.1333231401.git.jani@nikula.org>User-Agent:	Notmuch/0.12+70~g46e73fe (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu) <87d37mtms7.fsf@zancas.localnet>
User-Agent: Notmuch/0.11.1+222~ga47a98c (http://notmuchmail.org) Emacs/23.1.1
	(i686-pc-linux-gnu)
Date: Thu, 05 Apr 2012 12:00:50 +0000
Message-ID: <87ehs2xua5.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


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