On Tue, 2018-06-26 at 09:18 +0200, Michal Privoznik wrote:
> On 06/25/2018 07:11 PM, Andrea Bolognani wrote:
> > @@ -19803,9 +19803,12 @@ virDomainDefParseXML(xmlDocPtr xml,
> > tmp);
> > goto error;
> > }
> > - def->features[val] = value;
> > + def->hpt_resizing = (virDomainHPTResizing) value;
>
> This typecast seems useless. Also, pre-existing, the if() statement
> above is more verbose than it needs to be since
> VIR_DOMAIN_HPT_RESIZING_NONE is defined to be zero.
I agree the cast is mostly unnecessary here, but it doesn't cause
any issues either and as a personal preference I like being explicit
about this kind of conversion :)
As for the if() statement above, which looks like
int value = virDomainHPTResizingTypeFromString(tmp);
if (value < 0 || value == VIR_DOMAIN_HPT_RESIZING_NONE) {
...
you could definitely rewrite the condition as 'value <= 0' and get
the same behavior, but you would make the code more opaque as a
result.
Right now it doesn't take much effort to figure out what the code
above does: it very obviouly tries to convert a string to an enum,
and errors out if either the conversion fails entirely or the
returned value is one that we don't want the user to specify.
If rewritten, the two error conditions would be smushed together
and the developer would need to unpack them in their head, possibly
after looking up the virDomainHPTResizing enum and confirming 0
corresponds to a value that it would probably make sense to
blacklist when parsing the configuration.
The trade-off is very much not worth it just to save a few dozen
characters, in my opinion.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
