+
+/* Slightly evil macro that tests that two stanzas are equal without regarding
+ * their id, except that if one has an id and the other does not this is not
+ * considered a difference. It modifies the stanzas because I am lazy.
  */
-#define test_assert_stanzas_equal(s1, s2) \
+#define test_assert_stanzas_equal_no_id(s1, s2) \

The change here is adding "without regarding their id", and that's not true. The bit after the comma explains exactly how IDs are treated. So I don't think this improves the comment.

+wocky_xmpp_node_new (const char *name, const gchar *ns)
 {
   WockyXmppNode *result = g_slice_new0 (WockyXmppNode);
 
   result->name = g_strdup (name);
+  result->ns = (ns != NULL) ? g_quark_from_string (ns) : 0;

g_quark_from_string() is NULL-safe.


@@ -0,0 +1,85 @@
+/*
+ * wocky-node-tree.c - Source for WockyNodeTree
+ * Copyright (C) 2006 Collabora Ltd.

+ * wocky-node-tree.h - Header for WockyNodeTree
+ * Copyright (C) 2006,2010 Collabora Ltd.

I find that very hard to believe.

6af7e30332 adds:

+WockyNodeTree *wocky_node_tree_build (WockyNodeBuildTag first_tag,
+    ...) G_GNUC_NULL_TERMINATED;
+
+WockyNodeTree * wocky_node_tree_build_va (va_list ap);
+

but they're not implemented anywhere afaict.


+void
+wocky_xmpp_node_add_build (WockyXmppNode *node,
+    WockyNodeBuildTag first_tag,
+    ...)
+{
+  va_list ap;
+
+  va_start (ap, first_tag);
+  wocky_xmpp_node_add_build_va (node, ap);
+  va_end (ap);
+}


Doesn't this mean you're not passing first_tag to _build_va?


+/**
+ * wocky_node_add_build:
+ * @node: The node under which to add a new subtree
+ * @first_tag: The build tag for the first node
+ * @Varargs: the description of the stanza to build,
+ *  terminated with %NULL
+ *
+ * Add a node subtree to an existing parent node

There should be a full stop at the end of this sentence.

+ * Example:
+ * <example><programlisting>
+ * wocky_node_add_build (node,
+ *    '(', "body",
+ *        '$', "Telepathy rocks!",
+ *    ')',
+ *   NULL);
+ * </programlisting></example>
+ *
+ * <!-- -->* adds the following under the given node
+ * &lt;body&gt;
+ *    Telepathy rocks!
+ * &lt;/body&gt;
+ * *<!-- -->/
+ * </programlisting></example>
+ *
+ */

You close the <programlisting> and <example> twice. You can use |[ ... ]| as an abbreviation. I think the listing should end at the end of the C code, and the XML could be in its own block. <programlisting language="xml"> ?

(In reply to comment #1)
> +void
> +wocky_xmpp_node_add_build (WockyXmppNode *node,
> +    WockyNodeBuildTag first_tag,
> +    ...)
> +{
> +  va_list ap;
> +
> +  va_start (ap, first_tag);
> +  wocky_xmpp_node_add_build_va (node, ap);
> +  va_end (ap);
> +}
> 
> 
> Doesn't this mean you're not passing first_tag to _build_va?

It should have a smoke-test either way.
> + * <!-- -->* adds the following under the given node
> + * &lt;body&gt;
> + *    Telepathy rocks!
> + * &lt;/body&gt;
> + * *<!-- -->/
> + * </programlisting></example>
> + *
> + */
> 
> You close the <programlisting> and <example> twice. You can use |[ ... ]| as an
> abbreviation. I think the listing should end at the end of the C code, and the
> XML could be in its own block. <programlisting language="xml"> ?

(In reply to comment #1)
> +
> +/* Slightly evil macro that tests that two stanzas are equal without regarding
> + * their id, except that if one has an id and the other does not this is not
> + * considered a difference. It modifies the stanzas because I am lazy.
>   */
> -#define test_assert_stanzas_equal(s1, s2) \
> +#define test_assert_stanzas_equal_no_id(s1, s2) \
> 
> The change here is adding "without regarding their id", and that's not true.
> The bit after the comma explains exactly how IDs are treated. So I don't think
> this improves the comment.

revert that change, the function confused me more then i thought :)

> +wocky_xmpp_node_new (const char *name, const gchar *ns)
>  {
>    WockyXmppNode *result = g_slice_new0 (WockyXmppNode);
> 
>    result->name = g_strdup (name);
> +  result->ns = (ns != NULL) ? g_quark_from_string (ns) : 0;
> 
> g_quark_from_string() is NULL-safe.

fixed

> @@ -0,0 +1,85 @@
> +/*
> + * wocky-node-tree.c - Source for WockyNodeTree
> + * Copyright (C) 2006 Collabora Ltd.
> 
> + * wocky-node-tree.h - Header for WockyNodeTree
> + * Copyright (C) 2006,2010 Collabora Ltd.
> 
> I find that very hard to believe.

dates updated to be ranges

> 6af7e30332 adds:
> 
> +WockyNodeTree *wocky_node_tree_build (WockyNodeBuildTag first_tag,
> +    ...) G_GNUC_NULL_TERMINATED;
> +
> +WockyNodeTree * wocky_node_tree_build_va (va_list ap);
> +
> 
> but they're not implemented anywhere afaict.

yup nuked them

> +void
> +wocky_xmpp_node_add_build (WockyXmppNode *node,
> +    WockyNodeBuildTag first_tag,
> +    ...)
> +{
> +  va_list ap;
> +
> +  va_start (ap, first_tag);
> +  wocky_xmpp_node_add_build_va (node, ap);
> +  va_end (ap);
> +}
> 
> 
> Doesn't this mean you're not passing first_tag to _build_va?

yeah it's broken, fixed and test added

> +/**
> + * wocky_node_add_build:
> + * @node: The node under which to add a new subtree
> + * @first_tag: The build tag for the first node
> + * @Varargs: the description of the stanza to build,
> + *  terminated with %NULL
> + *
> + * Add a node subtree to an existing parent node
> 
> There should be a full stop at the end of this sentence.

. added

> + * Example:
> + * <example><programlisting>
> + * wocky_node_add_build (node,
> + *    '(', "body",
> + *        '$', "Telepathy rocks!",
> + *    ')',
> + *   NULL);
> + * </programlisting></example>
> + *
> + * <!-- -->* adds the following under the given node
> + * &lt;body&gt;
> + *    Telepathy rocks!
> + * &lt;/body&gt;
> + * *<!-- -->/
> + * </programlisting></example>
> + *
> + */
> 
> You close the <programlisting> and <example> twice. You can use |[ ... ]| as an
> abbreviation. I think the listing should end at the end of the C code, and the
> XML could be in its own block. <programlisting language="xml"> ?

slightly tweaked, also tweaked the doc generation as this didn't even end up there

full speed ahead