Re: [Qemu-devel] [PATCH trival] vl.c: clean up code

[Chen Gang]

On 03/31/2014 11:49 PM, Markus Armbruster wrote:
> Chen Gang <address@hidden> writes:
> 
>> in get_boot_device()
>>
>>  - remove 'res' to simplify code
>>
>> in main():
>>
>>  - remove useless 'continue'.
>>
>>  - in main switch():
>>
>>    - remove or adjust all useless 'break'.
>>
>>    - remove useless '{' and '}'.
>>
>>  - use assignment directly to replace useless 'args'
>>    (which is defined in the middle of code block).
> 
> Suggest to have one patch per item in this list.
> 

OK, thanks. I will/should send again in this week (within 2014-04-06).


[...]
>> @@ -3204,11 +3201,11 @@ int main(int argc, char **argv, char **envp)
>>              case QEMU_OPTION_curses:
>>  #ifdef CONFIG_CURSES
>>                  display_type = DT_CURSES;
>> +                break;
>>  #else
>>                  fprintf(stderr, "Curses support is disabled\n");
>>                  exit(1);
>>  #endif
>> -                break;
>>              case QEMU_OPTION_portrait:
>>                  graphic_rotate = 90;
>>                  break;
> 
> I'm not sure eliding the break after exit is worthwhile.
> 
> In theory, it could cause false positives with tools smart enough to
> diagnose fall through without comment, but too stupid to see that exit()
> never returns.  No idea whether such tools exist.
> 
> The missing break might give human readers slight pause, until they
> recognize exit.  Especially with such ifdeffery.
>

That sounds reasonable to me.

"removing 'break' after exit()" will not be good for C code readers:
exit() is not like 'return' which can get enough focus by C code readers
(especially in 'vim' or other latest editor).

How about to let 'return' instead of exit() in main(), and also remove
useless 'break'? Welcome any additional suggestions, discussions or
completions, thanks.


[...]
>> @@ -4371,15 +4363,13 @@ int main(int argc, char **argv, char **envp)
>>  
>>      qdev_machine_init();
>>  
>> -    QEMUMachineInitArgs args = { .machine = machine,
>> -                                 .ram_size = ram_size,
>> -                                 .boot_order = boot_order,
>> -                                 .kernel_filename = kernel_filename,
>> -                                 .kernel_cmdline = kernel_cmdline,
>> -                                 .initrd_filename = initrd_filename,
>> -                                 .cpu_model = cpu_model };
>> -
>> -    current_machine->init_args = args;
>> +    current_machine->init_args.machine = machine;
>> +    current_machine->init_args.ram_size = ram_size;
>> +    current_machine->init_args.boot_order = boot_order;
>> +    current_machine->init_args.kernel_filename = kernel_filename;
>> +    current_machine->init_args.kernel_cmdline = kernel_cmdline;
>> +    current_machine->init_args.initrd_filename = initrd_filename;
>> +    current_machine->init_args.cpu_model = cpu_model;
>>      machine->init(&current_machine->init_args);
>>  
>>      audio_init();
> 
> I agree with dropping useless variable args.  However, you don't have to
> replace the assignment of a compound literal by multiple simple
> assignments for that.  You could write
> 
>     current_machine->init_args = (QEMUMachineInitArgs){
>         .machine = machine,
>         .ram_size = ram_size,
>         .boot_order = boot_order,
>         .kernel_filename = kernel_filename,
>         .kernel_cmdline = kernel_cmdline,
>         .initrd_filename = initrd_filename,
>         .cpu_model = cpu_model };
> 
> Not exactly the same; it this implicitly zeroes members not mentioned.
> No such members exist now.
> 

That sounds good to me, I will/should send related patch v2.


Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed