[libvirt] [libvirt-glib PATCHv2 4/5] Add gvir_config_object_foreach_child helper

[Christophe Fergeau]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120105/a54613ac/attachment-0001.sig>