[PATCH 3/4] Utility function to seek in MIME trees in depth-first order.
Austin Clements
amdragon at MIT.EDU
Fri Dec 23 19:46:19 PST 2011
Quoth Dmitry Kurochkin on Dec 10 at 3:43 pm:
> On Fri, 9 Dec 2011 14:54:27 -0500, Austin Clements <amdragon at 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
More information about the notmuch
mailing list