[libvirt] [PATCH 0/2]: Cleanup driver initializers

[Laine Stump]

On 03/30/2009 08:09 AM, Chris Lalancette wrote:
> All,
>      This is a small series to cleanup the internal driver initializers.  The
> drivers were using an annoying combination of C-99 style initialization and
> old-style (C89?) initialization.  On top of that, the indentation on some of
> these was wacky, making it even harder to read.  After some discussion with
> DanB, we determined to go with the old-style initializers because it is sort of
> a self-documenting TODO list.  
>   

I see your point about the TODO list (since the old style initializers 
will throw a compiler error if you add a field to a struct and fail to 
initialize it everywhere, forcing you to at least put a "NULL," line 
in), while using the new style causes all unspecified fields to be 
auto-initialized to 0/NULL), but wanted to share my experience a couple 
weeks ago when attempting to understand the code by tracing through by 
hand (well, with emacs and xcscope.el ;-). I actually changed some 
struct initializations in my local directory in the opposite direction - 
from old style to new style (called "designated initializers", I found 
out). I did this because it makes it *much* easier to find your way if 
you're using something like cscope, which ignores comments.

As an example, when trying to figure out how virNetworkCreateXML works, 
my first step was to look at that function and see that it calls 
conn->networkDriver->networkCreateXML, which is a function pointer in 
virnetworkDriver. So, to see what actual code that might lead to, I put 
the cursor over networkCreateXML, hit and it finds:

    *** src/driver.h:   
    <global>[480]                  virDrvNetworkCreateXML networkCreateXML;

    *** src/remote_internal.c:
    <global>[6907]                 .networkCreateXML = 
remoteNetworkCreateXML,

    *** src/libvirt.c:
    virNetworkCreateXML[4826]      if (conn->networkDriver && 
conn->networkDriver->networkCreateXML) {
    virNetworkCreateXML[4828]      ret = 
conn->networkDriver->networkCreateXML (conn, xmlDesc);


You notice that it finds the place where networkCreateXML is initialized 
to remoteNetworkCreateXML, because a deisgnated initializer was used 
there. What's missing, though, is the initialization in 
network_driver.c, where the old style is used (cscope doesn't index 
comments, and comments aren't reliable anyway - during my explorations 
I've found cut-pasted code that had unmodified pre-cut-paste comments 
that are incorrect).

Once I change the initialization of the virNetworkDriver struct in 
network_driver.c to use designated initializers, cscope can find the 
line, and I get a better picture of how the code works.

    *** src/network_driver.c:
    <global>[1419]                 .networkCreateXML = networkCreate,

Of course this is just one example, but I'm guessing the same would 
happen to other people in the same situation.

Another advantage I will point out to designated initializers is that 
there is never any chance of inadvertantly mixing up the order of the 
members of a struct when initializing an instance (very easy to do when 
many of the fields are initialized to NULL), and thus introducing a bug 
that is impossible to spot during a simple visual inspection.

On the down side of designated initializers (and it seems the big reason 
you chose to standardize on the old style), since structures instances 
that are initialized with designated initializers don't require 
explicitly setting each and every member (unspecified members are just 
filled with 0's), it's possible to leave something out, and not receive 
a complaint from the compiler. (Wouldn't it be nice if that was added to 
gcc? Then everyone could get what they wanted at the same time)

A partial bandaid to using old-style is to (as you've done) put the 
member name in a comment on the same line as the initial value. This 
won't help cscope, and isn't compiler-guaranteed to be correct, but at 
least tools (other than cscope :-() will find it. I would strongly 
encourage anyone using old-style initializers to follow this practice.