On 10/07/2013 08:16 AM, Peter Krempa wrote:
>>> + switch ((virDomainHostdevSubsysPciBackendType)
>>> + dev->source.subsys.u.pci.backend) {
>>
>> I'm assuming you're doing the typecast so that you're forced to have a
>> case for every enum value. Do we have an official policy on that? I see
>> similar typecasts for switch statements in a few places, but I don't
>> know that I'm so fond of it...
>
> I started adding this on multiple places earlier. I don't know if
> there's a official strategy, but I tend to prefer it as I was bitten a
> few times by adding a new enum value and missed a few switches where I
> actually should handle those. That's why I'm adding the typecasts where
> possible.
I actually like it as well - any time you can make the compiler catch
your mistakes at compile time, rather than waiting until an obscure
error at runtime, is a win, even if the syntax is a bit awkward (casting
to force the correct type checking, and listing dead case labels to
appease the compiler).
>>
>> (Personally, I think if we're going to do enforce explicit listing of
>> all cases in switch statements for an attribute that has an enum
>> associated with it, it would be better to define the field with the
>> actual enum type rather than int - these are internal data structures
>> that are never passed from one machine to another, so it's not like
>> there would ever be any compatibility problem.)
>
> That would be a ideal global solution. Unfortunately as the
> SOMETypeFromString macroed functions return -1 on failure almost all
> variables holding the value got by this functions are declared as int
> rather than the appropriate enum value. To avoid the need for a separate
> intermediate variable we tend to declare the holding vars as int.
The cast inside the switch is a reasonable compromise, since it is often
easier to pass int around (and in public APIs, we _have_ to pass int
around - not all enums are private-use only).
>>> + break;
>>> +
>>> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT:
>>> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM:
>>> + case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST:
>>
>> Again - I dislike having these sentinel values showing up in switch
>> statements (searching the source I see it a lot), as it could lead
>> someone to believe that those are actually valid values. I may be
>> outvoted on this though :-)
>
> I think that the benefits of the compiler warning in case you add a new
> variable on places you might have forgotten outweigh a few occurences of
> unused type in the code.
Agreed. Maybe we should patch HACKING to capture the outcome of this
discussion.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list