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