From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
Subject: Re: [PATCH 3/4] Utility function to seek in MIME trees in depth-first
	order.
In-Reply-To: <1323460468-4030-4-git-send-email-amdragon@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>
User-Agent: Notmuch/0.10.2+82~g96a629c (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Sat, 10 Dec 2011 15:43:29 +0400
Message-ID: <87hb187iu6.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

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().

> +    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;

> +	return node;
> +
> +    *n = *n - 1;

Perhaps *n -= 1?  Or even --(*n)?

> +    for (i = 0; i < node->children && !ret; i++) {

Consider s/i++/++i/.

Regards,
  Dmitry

> +	mime_node_t *child = mime_node_child (node, i);
> +	ret = _mime_node_seek_dfs_walk (child, n);
> +	if (!ret)
> +	    talloc_free (child);
> +    }
> +    return ret;
> +}
> +
> +mime_node_t *
> +mime_node_seek_dfs (mime_node_t *node, int n)
> +{
> +    return _mime_node_seek_dfs_walk (node, &n);
> +}
> diff --git a/notmuch-client.h b/notmuch-client.h
> index fce1187..f215d4b 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -318,5 +318,10 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
>  mime_node_t *
>  mime_node_child (const mime_node_t *parent, int child);
>  
> +/* Return the nth child of node in a depth-first traversal.  If n is
> + * 0, returns node itself.  Returns NULL if there is no such part. */
> +mime_node_t *
> +mime_node_seek_dfs (mime_node_t *node, int n);
> +
>  #include "command-line-arguments.h"
>  #endif
> -- 
> 1.7.7.3
> 
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch