[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 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.

[...]

> 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

[...]

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.

ACK with the nit fixed. But, please, after the rebase (probably
tomorrow?), run the syntax-check and include all necessary changes to
make it pass (in case there are any), thanks.

Martin

P.S.: Again, if anyone has a look at the second patch in this series,
I'd appreciate that, since I'm not strong in perl.  However if it
doesn't get a review, I'll try to do what I can.


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