[libvirt] beginner project idea: enum scrubbing [was: Fwd: Start contributing with Libvirt (Finding a mentor or helpful tips)]

On 03/31/2014 03:43 PM, Eric Blake wrote: Here's an idea for a beginner's project. Very grunt work, but you mostly need time and patience, and not much understanding of the code base (I'd do it myself, if I had the time): We are inconsistent on how we declare enums. In our public headers, we made it a point to do: typedef enum { ... } virName; as it allows clients to use just 'virName' instead of 'enum virName' when referring to a variable holding an enum value. However, we also explicitly use 'int' instead of 'virName' for any API where an enum is passed in; this is intentional, because some compilers have flags that can change ABI if you pass enum types (if all values of an enum fit in a char, then the compiler can expose options to treat it as either a char or as an int, where the width chosen then affects how other code must link to that struct/function), whereas using 'int' leaves us with a stable ABI regardless of whether the client code played with those compiler flags. Besides, the C language is fairly lax about conversion in either direction between int and enum, unlike C++. Meanwhile, in our internal headers, we often have: enum virName { ... }; which means we HAVE to use 'enum virName' when referring to a variable holding one of those values. What's more, we have tended to carry over the public header notion of using 'int' instead of the enum type, so in util/ and conf/ you'll see lots of headers with struct members such as 'int type; /* enum virName */'. But this is stupid - our internal headers are NOT tied by ABI compatibility issues. And when seeing a function prototype, foo(int type) is a lot less instructive than foo(virName type), where type is supposed to be an enum value. Plus, we have lately been using the coding style of 'switch ((enum virName)expr) {...}' with no default: clause, to let the compiler enforce that we've covered all the enum values; but the cast in that switch is redundant if expr already has the proper enum type to begin with (and I'm a fan of removing redundant casts). I'd really like to see this cleaned up to use the typedef declaration style throughout our codebase, cleaning up internal headers to directly use enum types in both struct declarations and function declarations (where _only_ the public headers are forced to remain with int for ABI reasons), and checking that switch statements that try to cover all possible enum values use a consistent style that best aids the compiler. Ideally, we'd also implement some syntax check rules to enforce the style in the future; that part is trickier for a newcomer, but I don't mind helping on that front (and to some degree, tasks like this are sometimes easier if you write the checker first then hack the codebase until the checker no longer warns, then rebase the code into small enough patches with the checker last). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org