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@xxxxxxxxxx> 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
Attachment:
pgpd1eonluTQK.pgp
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list