On 10/08/2014 04:10 PM, Anirban Chakraborty wrote: >> Needs rework after my comments on 1/2. I also wonder if this should >> just be folded in to that patch, and/or made into a switch statement >> where the compiler forces us to think about any future >> VIR_DOMAIN_NET_TYPE_* additions on whether they should return true or >> false. >> bool virNetDevSupportBandwidth(virDomainNetType type) >> { >> switch (type) { >> case VIR_DOMAIN_NET_TYPE_BRIDGE: >> case VIR_DOMAIN_NET_TYPE_NETWORK: >> case VIR_DOMAIN_NET_TYPE_DIRECT: >> case VIR_DOMAIN_NET_TYPE_ETHERNET: >> return true; >> case VIR_DOMAIN_NET_TYPE_USER: >> case VIR_DOMAIN_NET_TYPE_VHOSTUSER: >> case VIR_DOMAIN_NET_TYPE_SERVER: >> case VIR_DOMAIN_NET_TYPE_CLIENT: >> case VIR_DOMAIN_NET_TYPE_MCAST: >> case VIR_DOMAIN_NET_TYPE_INTERNAL: >> case VIR_DOMAIN_NET_TYPE_HOSTDEV: >> case VIR_DOMAIN_NET_TYPE_LAST: >> /* cover all enums to appease the compiler */ ; >> } >> return false; >> } > > We could have the function defined with a switch, however, instead of > listing all the types, I could return false from the default case. > > Something like: > switch (type) { > case VIR_DOMAIN_NET_TYPE_BRIDGE: > case VIR_DOMAIN_NET_TYPE_NETWORK: > case VIR_DOMAIN_NET_TYPE_DIRECT: > case VIR_DOMAIN_NET_TYPE_ETHERNET: > return true; > > default; > return false; Alas, using a default case means the compiler can no longer tell you about missed cases. I wrote my example to intentionally spell out ALL cases without a default, in order to ensure the compiler will loudly warn if we add another enum in the future.

On 10/09/2014 12:21 AM, Anirban Chakraborty wrote: >> Alas, using a default case means the compiler can no longer tell you >> about missed cases. I wrote my example to intentionally spell out ALL >> cases without a default, in order to ensure the compiler will loudly >> warn if we add another enum in the future. > > I understood the intention. However, I was aiming for a compact code. If > we add additional enums for network type, we should be adding those >types > in the above switch provided the new interface type supports bandwidth. >I > do not know if we should prevent future coding error by elaborating the > current code. Forgive me, as I do not know the coding convention for > libvirt. If this is the standard practice, I¹ll change the above switch >as > per your wish. Hmm - we really ought to add a segment to HACKING that mentions the trick. The thing to remember is that gcc is able to warn for any enum used as a switch statement where there are only 'case FOO:' labels and no 'default:' label, and where not all the enum values are covered. So code-maintenance wise, if you have an enum that might grow a value in the future, and want to make sure any addition of a new value gets automatically flagged as to all places of code that need to think about behavior related to that new value, then using a switch with no default lets the compiler help you find those places.

On 10/10/2014 10:59 AM, Anirban Chakraborty wrote: > The list is really very long to put all in here. If we have to do what >you > suggested, then we¹d be changing all such usage of switch statements, > which is not trivial, I believe. The point of adding it to HACKING is to encourage new code to abide by the standard, and not necessarily to retrofit existing code. And you are correct that existing code doesn't always use enum type-safety compiler guarantees - which makes the enum that much harder to modify later if we ever add enum values. But in some cases, it is fairly obvious that we don't plan to add any enum values, in which case a simple if statement or use of a default label is fine. It's a case-by-case judgment call of what makes the code easier to maintain.