[PATCH 3/4] Utility function to seek in MIME trees in depth-first order.
Dmitry Kurochkin
dmitry.kurochkin at gmail.com
Sun Dec 25 15:39:57 PST 2011
On Fri, 23 Dec 2011 22:46:19 -0500, Austin Clements <amdragon at 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 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 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
More information about the notmuch
mailing list