[PATCH v2 1/3] mime node: Record depth-first part numbers
Austin Clements
amdragon at MIT.EDU
Mon Jan 23 15:15:35 PST 2012
Quoth Dmitry Kurochkin on Jan 24 at 2:19 am:
> On Sun, 22 Jan 2012 21:31:11 -0500, Austin Clements <amdragon at MIT.EDU> wrote:
> > This makes the part numbers readily accessible to formatters.
> > Hierarchical part numbering would be a more natural and efficient fit
> > for MIME and may be the way to go in the future, but depth-first
> > numbering maintains compatibility with what we currently do.
>
> The patch looks good except for few minor comments below. I do not
> think that we need another review for the next patch version.
>
> > ---
> > mime-node.c | 37 ++++++++++++++++++++++++++++++++++---
> > notmuch-client.h | 14 +++++++++++++-
> > 2 files changed, 47 insertions(+), 4 deletions(-)
> >
> > diff --git a/mime-node.c b/mime-node.c
> > index 27077f7..025c537 100644
> > --- a/mime-node.c
> > +++ b/mime-node.c
> > @@ -112,6 +112,10 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
> > root->nchildren = 1;
> > root->ctx = mctx;
> >
> > + root->part_num = 0;
> > + root->next_child = 0;
> > + root->next_part_num = 1;
> > +
>
> For consistency, we should either initialize root->parent to NULL
> explicitly or remove part_num and next_child initialization.
>
> > *root_out = root;
> > return NOTMUCH_STATUS_SUCCESS;
> >
> > @@ -137,7 +141,7 @@ _signature_validity_free (GMimeSignatureValidity **proxy)
> > #endif
> >
> > static mime_node_t *
> > -_mime_node_create (const mime_node_t *parent, GMimeObject *part)
> > +_mime_node_create (mime_node_t *parent, GMimeObject *part)
> > {
> > mime_node_t *node = talloc_zero (parent, mime_node_t);
> > GError *err = NULL;
> > @@ -150,6 +154,8 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
> > talloc_free (node);
> > return NULL;
> > }
> > + node->parent = parent;
> > + node->part_num = node->next_part_num = -1;
>
> Same here: if we initialize next_child to zero explicitly in
> mime_node_open(), we should do it here as well.
Both done.
> >
> > /* Deal with the different types of parts */
> > if (GMIME_IS_PART (part)) {
> > @@ -267,9 +273,10 @@ _mime_node_create (const mime_node_t *parent, GMimeObject *part)
> > }
> >
> > mime_node_t *
> > -mime_node_child (const mime_node_t *parent, int child)
> > +mime_node_child (mime_node_t *parent, int child)
> > {
> > GMimeObject *sub;
> > + mime_node_t *node;
> >
> > if (!parent || child < 0 || child >= parent->nchildren)
> > return NULL;
> > @@ -287,7 +294,31 @@ mime_node_child (const mime_node_t *parent, int child)
> > INTERNAL_ERROR ("Unexpected GMimeObject type: %s",
> > g_type_name (G_OBJECT_TYPE (parent->part)));
> > }
> > - return _mime_node_create (parent, sub);
> > + node = _mime_node_create (parent, sub);
> > +
> > + if (child == parent->next_child && parent->next_part_num != -1) {
> > + /* We're traversing in depth-first order. Record the child's
> > + * depth-first numbering. */
> > + node->part_num = parent->next_part_num;
> > + node->next_part_num = node->part_num + 1;
> > +
> > + /* Drop the const qualifier because these are internal fields
> > + * whose mutability doesn't affect the interface. */
>
> The comment is no longer relevant, please remove.
>
> > + parent->next_child++;
> > + parent->next_part_num = -1;
> > +
> > + if (node->nchildren == 0) {
> > + /* We've reached a leaf, so find the parent that has more
> > + * children and set it up to number its next child. */
> > + mime_node_t *it = node;
> > + while (it && it->next_child == it->nchildren)
>
> It seems that it should be initialized to node->parent, because
> node->next_child is always 0.
Either works. I started at node because it seemed like a more
fundamental base case, but perhaps that just makes this code even more
obtuse.
> Just curious, does "it" stands for "iterator"? I would prefer just "i"
> or "iter" :)
"it" is a C++ habit. I changed it to "iter".
> > + it = it->parent;
> > + if (it)
> > + it->next_part_num = node->part_num + 1;
> > + }
> > + }
>
> Austin, I trust you that this code works :) Though I hope we will get
> to hierarchical part numbering one day and it would simplify this code.
It would simplify it down to one line, in fact. Code simplification
aside, I think hierarchical numbering is the right thing to do. But
that's for another day.
> Regards,
> Dmitry
>
> > +
> > + return node;
> > }
> >
> > static mime_node_t *
> > diff --git a/notmuch-client.h b/notmuch-client.h
> > index 9c1d383..abfe5d3 100644
> > --- a/notmuch-client.h
> > +++ b/notmuch-client.h
> > @@ -297,6 +297,13 @@ typedef struct mime_node {
> > /* The number of children of this part. */
> > int nchildren;
> >
> > + /* The parent of this node or NULL if this is the root node. */
> > + struct mime_node *parent;
> > +
> > + /* The depth-first part number of this child if the MIME tree is
> > + * being traversed in depth-first order, or -1 otherwise. */
> > + int part_num;
> > +
> > /* True if decryption of this part was attempted. */
> > notmuch_bool_t decrypt_attempted;
> > /* True if decryption of this part's child succeeded. In this
> > @@ -324,6 +331,11 @@ typedef struct mime_node {
> > /* Internal: For successfully decrypted multipart parts, the
> > * decrypted part to substitute for the second child. */
> > GMimeObject *decrypted_child;
> > +
> > + /* Internal: The next child for depth-first traversal and the part
> > + * number to assign it (or -1 if unknown). */
> > + int next_child;
> > + int next_part_num;
> > } mime_node_t;
> >
> > /* Construct a new MIME node pointing to the root message part of
> > @@ -356,7 +368,7 @@ mime_node_open (const void *ctx, notmuch_message_t *message,
> > * an error message on stderr).
> > */
> > mime_node_t *
> > -mime_node_child (const mime_node_t *parent, int child);
> > +mime_node_child (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. */
>
--
Austin Clements MIT/'06/PhD/CSAIL
amdragon at mit.edu http://web.mit.edu/amdragon
Somewhere in the dream we call reality you will find me,
searching for the reality we call dreams.
More information about the notmuch
mailing list