Re: [libvirt] [PATCH] Don't use enums in TPM struct fields

On Wed, Jun 06, 2018 at 05:47:00PM +0100, Daniel P. Berrangé wrote: or anywhere else What I proposed during review was assigning the result to an int variable before filling the version: https://www.redhat.com/archives/libvir-list/2018-June/msg00114.html If 'getting a compiler warning' means easy to miss, then we should not have bothered with all those switch enumerations and virEnumRangeError stuff. [... lots of lines that aren't required to fix the build ;) ...] This deprives us of the -Wswitch-enum warning on all compilers because some don't detect the bogus negative value comparison. And the comment has even less power than the clang warning. So: 1. Is it actually worth the trouble to store enum values in typedef'd enums? 2. If so, can we make TypeFromString usage less cumbersome? The fact that the compiler can choose different types rules out returning the parsed value via a pointer. A macro cannot both return a value and declare the temporary int value (can it?), so doing: if (typeFromString(def->version, virDomainTPMVersion, version) < 0) won't work either. I can imagine separating the check and the conversion: if (!virDomainTPMVersionTypeCheck(version)) { virReportError(); goto cleanup; } def->version = virDomainTPMVersionTypeFromString(version) But not even clang will catch the missing check and it would go through the array twice. Alternatively, we can hide some control flow into the macro, which will look ugly in the middle of a function: #define VIR_ENUM_PARSE_GOTO(ret, name, type, label) do { int val = name ## TypeFromString(type); if (val < 0) { virReportError(VIR_ERR_XML_ERROR, "generic parse error"); goto label; } ret = val; } while(0); VIR_ENUM_PARSE_GOTO(def->version, virDomainTPMVersion, version, cleanup); Hopefully someone has a better idea. Jano