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