[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 1/2] Remove spurious whitespace between function name & open brackets



On Thu, Nov 01, 2012 at 03:00:40PM +0100, Martin Kletzander wrote:
> On 11/01/2012 11:53 AM, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange redhat com>
> > 
> > The libvirt coding standard is to use 'function(...args...)'
> > instead of 'function (...args...)'. A non-trivial number of
> > places did not follow this rule and are fixed in this patch.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange redhat com>
> > ---
> 
> It would be easier to review if it was split into few more commits, but
> I'm going for it anyway.
> 
> [...]
> 
> > diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
> > index e691c04..ca8cd92 100644
> > --- a/src/cpu/cpu_x86.c
> > +++ b/src/cpu/cpu_x86.c
> > @@ -548,16 +548,16 @@ x86VendorLoad(xmlXPathContextPtr ctxt,
> >      }
> >  
> >      vendor->cpuid.function = 0;
> > -    vendor->cpuid.ebx = (string[0]       ) |
> > -                        (string[1]  <<  8) |
> > -                        (string[2]  << 16) |
> > -                        (string[3]  << 24);
> > -    vendor->cpuid.edx = (string[4]       ) |
> > -                        (string[5]  <<  8) |
> > -                        (string[6]  << 16) |
> > -                        (string[7]  << 24);
> > -    vendor->cpuid.ecx = (string[8]       ) |
> > -                        (string[9]  <<  8) |
> > +    vendor->cpuid.ebx = (string[0]) |
> > +                        (string[1] << 8) |
> > +                        (string[2] << 16) |
> > +                        (string[3] << 24);
> > +    vendor->cpuid.edx = (string[4]) |
> > +                        (string[5] << 8) |
> > +                        (string[6] << 16) |
> > +                        (string[7] << 24);
> > +    vendor->cpuid.ecx = (string[8]) |
> > +                        (string[9] << 8) |
> >                          (string[10] << 16) |
> >                          (string[11] << 24);
> >  
> 
> Is this hunk (and similar) necessary?  IMHO it doesn't violate the
> syntax and it's a bit nicer to read.

There is whitespace before the closing ')' here which is
being killed.

> > diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c
> > index 350a8ec..2c0662f 100644
> > --- a/src/nwfilter/nwfilter_dhcpsnoop.c
> > +++ b/src/nwfilter/nwfilter_dhcpsnoop.c
> > @@ -1084,7 +1084,7 @@ virNWFilterSnoopDHCPOpen(const char *ifname, virMacAddr *mac,
> >  
> >      virMacAddrFormat(mac, macaddr);
> >  
> > -    if (dir == PCAP_D_IN /* from VM */) {
> > +    if (dir == PCAP_D_IN) /* from VM */) {
> 
> This change fails the build, remove one of the brackets

Opps, I changed this then reverted it, but obviously didn't
properly revert it.

> [...]
> 
> Just a question aside.  We don't care about this kind of syntax?
> 
> tools/virsh.h:197:    bool (*handler) (vshControl *, const vshCmd *);
> 
> I know it's even harder to algorithmically check this and I don't
> require it for this patch, I'm just curious.

I've not been able to find a reliable way to deal with function
prototype typedefs like this. If I could, then I'd clean them
too. This is one of the main reasons why I skipped all header
files in this cleanup

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]