Date: Fri, 23 Dec 2011 22:46:19 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
Subject: Re: [PATCH 3/4] Utility function to seek in MIME trees in
	depth-first order.
Message-ID: <20111224034603.GC1927@mit.edu>
References: <1323027100-10307-1-git-send-email-amdragon@mit.edu>
	<1323460468-4030-1-git-send-email-amdragon@mit.edu>
	<1323460468-4030-4-git-send-email-amdragon@mit.edu>
	<87hb187iu6.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87hb187iu6.fsf@gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Dmitry Kurochkin on Dec 10 at  3:43 pm:
> On Fri,  9 Dec 2011 14:54:27 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> > This function matches how we number parts for the --part argument to
> > show.  It will allow us to jump directly to the desired part, rather
> > than traversing the entire tree and carefully tracking whether or not
> > we're "in the zone".
> > ---
> >  mime-node.c      |   25 +++++++++++++++++++++++++
> >  notmuch-client.h |    5 +++++
> >  2 files changed, 30 insertions(+), 0 deletions(-)
> > 
> > diff --git a/mime-node.c b/mime-node.c
> > index a8e4a59..207818e 100644
> > --- a/mime-node.c
> > +++ b/mime-node.c
> > @@ -232,3 +232,28 @@ mime_node_child (const mime_node_t *parent, int child)
> >  			g_type_name (G_OBJECT_TYPE (parent->part)));
> >      }
> >  }
> > +
> > +static mime_node_t *
> > +_mime_node_seek_dfs_walk (mime_node_t *node, int *n)
> > +{
> > +    mime_node_t *ret = NULL;
> > +    int i;
> > +
> 
> Can we move declarations below the if (which does not need them)?  I
> always have troubles remembering if (recent enough) C standard allows
> that or it is a GCC extension.  FWIW in the previous patch there are
> declarations in the middle of a block, e.g.:
> 
> 	} else {
> 	    out->is_signed = TRUE;
>             ...
> 	    GMimeSignatureValidity *sig_validity = g_mime_multipart_signed_verify
> 		(GMIME_MULTIPART_SIGNED (part), out->ctx->cryptoctx, &err);
> 
> So either we can move these declarations to where they are needed, or we
> should fix it in _mime_node_create().

Since prevailing notmuch style seems to be top-declarations, I fixed
up _mime_node_create instead (personally I prefer C99-style
declarations, but *shrug*).

> > +    if (*n <= 0)
> 
> Comment for mime_node_seek_dfs() says that the function returns the node
> itself for n = 0, but does not say anything about n < 0.  I would expect
> the function to return NULL for n < 0.  In any case, the comment below
> should probably mention what happens for n < 0;

Good point.  I made it return NULL for n < 0.  I think this logically
falls under "Returns NULL if there is no such part."

> > +	return node;
> > +
> > +    *n = *n - 1;
> 
> Perhaps *n -= 1?  Or even --(*n)?

Changed to *n -= 1.

> > +    for (i = 0; i < node->children && !ret; i++) {
> 
> Consider s/i++/++i/.

notmuch uses i++ remarkably consistently, so I left this.

> Regards,
>   Dmitry