Summary: | WockyNodeTree and friends | ||
---|---|---|---|
Product: | Wocky | Reporter: | Will Thompson <will> |
Component: | General | Assignee: | Will Thompson <will> |
Status: | RESOLVED FIXED | QA Contact: | Telepathy bugs list <telepathy-bugs> |
Severity: | normal | ||
Priority: | medium | Keywords: | patch |
Version: | unspecified | ||
Hardware: | Other | ||
OS: | All | ||
URL: | http://git.collabora.co.uk/?p=user/sjoerd/wocky.git;a=shortlog;h=refs/heads/wockynodetree | ||
Whiteboard: | review+ | ||
i915 platform: | i915 features: |
Description
Will Thompson
2010-04-19 09:39:33 UTC
+ +/* 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 + * <body> + * Telepathy rocks! + * </body> + * *<!-- -->/ + * </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 > + * <body> > + * Telepathy rocks! > + * </body> > + * *<!-- -->/ > + * </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 > + * <body> > + * Telepathy rocks! > + * </body> > + * *<!-- -->/ > + * </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 |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.