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

Re: [libvirt] [PATCH] libxl: remove unnecessary curly braces



Eric Blake wrote:
> On 08/14/2013 03:41 PM, Jim Fehlig wrote:
>   
>> As per HACKING, remove some unneeded curly braces in the
>> libxl driver.
>> ---
>>
>> Noticed the unneeded braces while reviewing Chunyan's hostdev
>> passthrough series.  Not sure if this qualifies as trivial
>> enough to just push, but best for a quick review anyhow to
>> ensure I didn't botch something.
>>
>>  src/libxl/libxl_conf.c   | 24 ++++++++----------------
>>  src/libxl/libxl_driver.c | 27 ++++++++++++---------------
>>  2 files changed, 20 insertions(+), 31 deletions(-)
>>
>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>> index 827dfdd..4362e62 100644
>> --- a/src/libxl/libxl_conf.c
>> +++ b/src/libxl/libxl_conf.c
>> @@ -244,12 +244,10 @@ libxlMakeCapabilitiesInternal(virArch hostarch,
>>              }
>>  
>>              /* Search for existing matching (model,hvm) tuple */
>> -            for (i = 0; i < nr_guest_archs; i++) {
>> +            for (i = 0; i < nr_guest_archs; i++)
>>                  if ((guest_archs[i].arch == arch) &&
>> -                    guest_archs[i].hvm == hvm) {
>> +                    guest_archs[i].hvm == hvm)
>>                      break;
>> -                }
>> -            }
>>     
>
> The inner brace removal is okay, but I tend to use {} on anything that
> occupies more than one line (even if only one statement), particularly
> if more than one of the nested lines have the same indentation (as is
> the case with the split-line if).  That means I think the outer braces
> should stay.
>   

Yes, good point.  Also, looking at that section of HACKING again, it
says the braces can be omitted "only when that
body occupies a single line".

>   
>>  
>>              /* Too many arch flavours - highly unlikely ! */
>>              if (i >= ARRAY_CARDINALITY(guest_archs))
>> @@ -690,10 +688,9 @@ libxlMakeDiskList(virDomainDefPtr def, libxl_domain_config *d_config)
>>      if (VIR_ALLOC_N(x_disks, ndisks) < 0)
>>          return -1;
>>  
>> -    for (i = 0; i < ndisks; i++) {
>> +    for (i = 0; i < ndisks; i++)
>>          if (libxlMakeDisk(l_disks[i], &x_disks[i]) < 0)
>>              goto error;
>> -    }
>>     
>
> This one's borderline - it's more than one line, but at different
> indentation, so it's not as visually difficult to spot.
>
> I can live with this patch as-is, so weak ACK in case you want to wait
> for any other opinions.
>   

I removed such changes as you noted above, where there were more than
one line, and pushed the patch.

Regards,
Jim


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