From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
To: Austin Clements <amdragon@MIT.EDU>
Subject: Re: [PATCH 3/4] Utility function to seek in MIME trees in depth-first
	order.
In-Reply-To: <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> <20111224034603.GC1927@mit.edu>
User-Agent: Notmuch/0.10.2+116~g4c449d4 (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Mon, 26 Dec 2011 03:39:57 +0400
Message-ID: <87pqfc1ar6.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: notmuch@notmuchmail.org
Precedence: list

On Fri, 23 Dec 2011 22:46:19 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> 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 there is any code which already uses C99-style declarations, then we
should use them in the new code IMO.  Perhaps whether to use C99-style
declarations or not should be a coding style requirement.

Regards,
  Dmitry

> > > +    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