Quoth Dmitry Kurochkin on Jan 24 at 2:19 am: > On Sun, 22 Jan 2012 21:31:11 -0500, Austin Clements <amdragon@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@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.