Re: [libvirt] [libvirt-glib PATCHv2 4/5] Add gvir_config_object_foreach_child helper
Hey,
Thanks for reviewing all these patches!
On Wed, Jan 04, 2012 at 10:34:15PM +0200, Zeeshan Ali (Khattak) wrote:
On Tue, Jan 3, 2012 at 4:50 PM, Christophe Fergeau
<cfergeau(a)redhat.com> wrote:
> When iterating over xmlNodePtr children to parse an XML document
> describing a libvirt configuration item, we want to ignore blank
> nodes that may have been added to make the initial XML document
> "more human readable". Since this will be useful in several places,
> move this code to a helper function.
Looks good, apart from the "more human readable" sarcasm. :)
It's not meant as a sarcasm, it's generally why some XML documents have
some extra white space, so I'm simply stating a fact :)
Oh and
a few small things:
> +static gboolean add_one_feature(xmlNodePtr node, gpointer opaque)
'feature' is singular so 'one_' is pretty much redundant here.
This is an habit I have for callbacks used with _foreach functions, I put a
"_one_" in the name to underline it's processing one element of the
collection that is being iterated over. I removed it here, and did the same
in the commit introducing _get_devices which has a add_one_device function.
> +void gvir_config_xml_foreach_child(xmlNodePtr node,
> + GVirConfigXmlNodeIterator it_func,
> + gpointer opaque)
I'd suggest using 'iter_func' or just 'func' as the parameter name
but this is entirely subjective so feel free to ignore this
suggestion. :)
Changed
> + for (it = node->children; it != NULL; it = it->next) {
> + gboolean cont;
> +
> + if (xmlIsBlankNode(it))
> + continue;
> + cont = it_func(it, opaque);
> + if (!cont)
> + break;
Since 'cont' is only used once and there is no big line (to not fit
in the 120 cols limit), I suggest not using it here.
I don't like hiding function calls in if() conditions like if (!it_func(it,
opaque)) break, that's why I used a variable, and I'd prefer to keep it
that way :)
Christophe
Attachments:
- attachment.sig (application/pgp-signature — 198 bytes)