| Message ID | 53382B89.9030301@gmail.com |
|---|---|
| State | New |
| Headers | show |
Hello Maintainers:
In main switch of main(), it contents several styles for "{...}" code block.
If it is necessary to use unique style within a function, please let me
know, I will/should clean up it. And also better to tell me which style
we need choose -- for me, I don't know which style is the best to Qemu.
Thanks.
On 03/30/2014 10:34 PM, Chen Gang wrote:
> 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).
>
>
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
> vl.c | 36 +++++++++++++-----------------------
> 1 file changed, 13 insertions(+), 23 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 9975e5a..9c733cb 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1188,18 +1188,16 @@ DeviceState *get_boot_device(uint32_t position)
> {
> uint32_t counter = 0;
> FWBootEntry *i = NULL;
> - DeviceState *res = NULL;
>
> if (!QTAILQ_EMPTY(&fw_boot_order)) {
> QTAILQ_FOREACH(i, &fw_boot_order, link) {
> if (counter == position) {
> - res = i->dev;
> - break;
> + return i->dev;
> }
> counter++;
> }
> }
> - return res;
> + return NULL;
> }
>
> /*
> @@ -3034,7 +3032,6 @@ int main(int argc, char **argv, char **envp)
> if (argv[optind][0] != '-') {
> /* disk image */
> optind++;
> - continue;
> } else {
> const QEMUOption *popt;
>
> @@ -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;
> @@ -3286,7 +3283,6 @@ int main(int argc, char **argv, char **envp)
> case QEMU_OPTION_audio_help:
> AUD_help ();
> exit (0);
> - break;
> case QEMU_OPTION_soundhw:
> select_soundhw (optarg);
> break;
> @@ -3296,7 +3292,6 @@ int main(int argc, char **argv, char **envp)
> case QEMU_OPTION_version:
> version();
> exit(0);
> - break;
> case QEMU_OPTION_m: {
> int64_t value;
> uint64_t sz;
> @@ -3638,11 +3633,10 @@ int main(int argc, char **argv, char **envp)
> olist = qemu_find_opts("machine");
> qemu_opts_parse(olist, "accel=tcg", 0);
> break;
> - case QEMU_OPTION_no_kvm_pit: {
> + case QEMU_OPTION_no_kvm_pit:
> fprintf(stderr, "Warning: KVM PIT can no longer be disabled "
> "separately.\n");
> break;
> - }
> case QEMU_OPTION_no_kvm_pit_reinjection: {
> static GlobalProperty kvm_pit_lost_tick_policy[] = {
> {
> @@ -3681,11 +3675,11 @@ int main(int argc, char **argv, char **envp)
> #ifdef CONFIG_VNC
> display_remote++;
> vnc_display = optarg;
> + break;
> #else
> fprintf(stderr, "VNC support is disabled\n");
> exit(1);
> #endif
> - break;
> case QEMU_OPTION_no_acpi:
> acpi_enabled = 0;
> break;
> @@ -3811,7 +3805,6 @@ int main(int argc, char **argv, char **envp)
> xen_mode = XEN_ATTACH;
> break;
> case QEMU_OPTION_trace:
> - {
> opts = qemu_opts_parse(qemu_find_opts("trace"), optarg, 0);
> if (!opts) {
> exit(1);
> @@ -3819,7 +3812,6 @@ int main(int argc, char **argv, char **envp)
> trace_events = qemu_opt_get(opts, "events");
> trace_file = qemu_opt_get(opts, "file");
> break;
> - }
> case QEMU_OPTION_readconfig:
> {
> int ret = qemu_read_config_file(optarg);
> @@ -3876,12 +3868,12 @@ int main(int argc, char **argv, char **envp)
> if (!opts) {
> exit(1);
> }
> + break;
> #else
> error_report("File descriptor passing is disabled on this "
> "platform");
> exit(1);
> #endif
> - break;
> case QEMU_OPTION_object:
> opts = qemu_opts_parse(qemu_find_opts("object"), optarg, 1);
> if (!opts) {
> @@ -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(¤t_machine->init_args);
>
> audio_init();
>
Chen Gang <gang.chen.5i5j@gmail.com> 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. > > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> > --- > vl.c | 36 +++++++++++++----------------------- > 1 file changed, 13 insertions(+), 23 deletions(-) > > diff --git a/vl.c b/vl.c > index 9975e5a..9c733cb 100644 > --- a/vl.c > +++ b/vl.c > @@ -1188,18 +1188,16 @@ DeviceState *get_boot_device(uint32_t position) > { > uint32_t counter = 0; > FWBootEntry *i = NULL; > - DeviceState *res = NULL; > > if (!QTAILQ_EMPTY(&fw_boot_order)) { > QTAILQ_FOREACH(i, &fw_boot_order, link) { > if (counter == position) { > - res = i->dev; > - break; > + return i->dev; > } > counter++; > } > } > - return res; > + return NULL; > } > > /* > @@ -3034,7 +3032,6 @@ int main(int argc, char **argv, char **envp) > if (argv[optind][0] != '-') { > /* disk image */ > optind++; > - continue; > } else { > const QEMUOption *popt; > > @@ -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. Dunno. > @@ -3286,7 +3283,6 @@ int main(int argc, char **argv, char **envp) > case QEMU_OPTION_audio_help: > AUD_help (); > exit (0); > - break; > case QEMU_OPTION_soundhw: > select_soundhw (optarg); > break; > @@ -3296,7 +3292,6 @@ int main(int argc, char **argv, char **envp) > case QEMU_OPTION_version: > version(); > exit(0); > - break; > case QEMU_OPTION_m: { > int64_t value; > uint64_t sz; > @@ -3638,11 +3633,10 @@ int main(int argc, char **argv, char **envp) > olist = qemu_find_opts("machine"); > qemu_opts_parse(olist, "accel=tcg", 0); > break; > - case QEMU_OPTION_no_kvm_pit: { > + case QEMU_OPTION_no_kvm_pit: > fprintf(stderr, "Warning: KVM PIT can no longer be disabled " > "separately.\n"); > break; > - } > case QEMU_OPTION_no_kvm_pit_reinjection: { > static GlobalProperty kvm_pit_lost_tick_policy[] = { > { > @@ -3681,11 +3675,11 @@ int main(int argc, char **argv, char **envp) > #ifdef CONFIG_VNC > display_remote++; > vnc_display = optarg; > + break; > #else > fprintf(stderr, "VNC support is disabled\n"); > exit(1); > #endif > - break; > case QEMU_OPTION_no_acpi: > acpi_enabled = 0; > break; > @@ -3811,7 +3805,6 @@ int main(int argc, char **argv, char **envp) > xen_mode = XEN_ATTACH; > break; > case QEMU_OPTION_trace: > - { > opts = qemu_opts_parse(qemu_find_opts("trace"), optarg, 0); > if (!opts) { > exit(1); > @@ -3819,7 +3812,6 @@ int main(int argc, char **argv, char **envp) > trace_events = qemu_opt_get(opts, "events"); > trace_file = qemu_opt_get(opts, "file"); > break; > - } > case QEMU_OPTION_readconfig: > { > int ret = qemu_read_config_file(optarg); > @@ -3876,12 +3868,12 @@ int main(int argc, char **argv, char **envp) > if (!opts) { > exit(1); > } > + break; > #else > error_report("File descriptor passing is disabled on this " > "platform"); > exit(1); > #endif > - break; > case QEMU_OPTION_object: > opts = qemu_opts_parse(qemu_find_opts("object"), optarg, 1); > if (!opts) { > @@ -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(¤t_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. Matter of taste, I guess.
On 03/31/2014 11:49 PM, Markus Armbruster wrote: > Chen Gang <gang.chen.5i5j@gmail.com> 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(¤t_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 <gang.chen.5i5j@gmail.com> writes: > On 03/31/2014 11:49 PM, Markus Armbruster wrote: >> Chen Gang <gang.chen.5i5j@gmail.com> 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). No rush :) > [...] >>> @@ -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. When main() is short, return instead of exit() is just fine with me. But when it's as long as this one, I prefer exit() to make the process termination locally obvious. I think the code is okay as it is. [...]
On 04/01/2014 04:13 PM, Markus Armbruster wrote: > Chen Gang <gang.chen.5i5j@gmail.com> writes: > >> On 03/31/2014 11:49 PM, Markus Armbruster wrote: >>> Chen Gang <gang.chen.5i5j@gmail.com> 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). > > No rush :) > Yeah, more discussions, and more thinking of, before implementations. >> [...] >>>> @@ -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. > > When main() is short, return instead of exit() is just fine with me. > But when it's as long as this one, I prefer exit() to make the process > termination locally obvious. > > I think the code is okay as it is. > I guess, the root cause is the main() is too long, and need be split into several small functions, then use 'return' instead of exit(), and then remove "'break' after exit()". And after split into small functions, we also can by pass the several "{...}" styles within switch() in main() -- let the related code block be in individual functions. Thanks.
Chen Gang <gang.chen.5i5j@gmail.com> writes: > Hello Maintainers: > > In main switch of main(), it contents several styles for "{...}" code block. > > If it is necessary to use unique style within a function, please let me > know, I will/should clean up it. And also better to tell me which style > we need choose -- for me, I don't know which style is the best to > Qemu. The correct block style is described in CODING_STYLE Section 4. However chunks of the code base violate the style guidelines and should be cleaned up as other fixes are made. > > > Thanks. > > On 03/30/2014 10:34 PM, Chen Gang wrote: >> 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). >> >> >> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> >> --- >> vl.c | 36 +++++++++++++----------------------- >> 1 file changed, 13 insertions(+), 23 deletions(-) >> >> diff --git a/vl.c b/vl.c >> index 9975e5a..9c733cb 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -1188,18 +1188,16 @@ DeviceState *get_boot_device(uint32_t position) >> { >> uint32_t counter = 0; >> FWBootEntry *i = NULL; >> - DeviceState *res = NULL; >> >> if (!QTAILQ_EMPTY(&fw_boot_order)) { >> QTAILQ_FOREACH(i, &fw_boot_order, link) { >> if (counter == position) { >> - res = i->dev; >> - break; >> + return i->dev; >> } >> counter++; >> } >> } >> - return res; >> + return NULL; >> } >> >> /* >> @@ -3034,7 +3032,6 @@ int main(int argc, char **argv, char **envp) >> if (argv[optind][0] != '-') { >> /* disk image */ >> optind++; >> - continue; >> } else { >> const QEMUOption *popt; >> >> @@ -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; >> @@ -3286,7 +3283,6 @@ int main(int argc, char **argv, char **envp) >> case QEMU_OPTION_audio_help: >> AUD_help (); >> exit (0); >> - break; >> case QEMU_OPTION_soundhw: >> select_soundhw (optarg); >> break; >> @@ -3296,7 +3292,6 @@ int main(int argc, char **argv, char **envp) >> case QEMU_OPTION_version: >> version(); >> exit(0); >> - break; >> case QEMU_OPTION_m: { >> int64_t value; >> uint64_t sz; >> @@ -3638,11 +3633,10 @@ int main(int argc, char **argv, char **envp) >> olist = qemu_find_opts("machine"); >> qemu_opts_parse(olist, "accel=tcg", 0); >> break; >> - case QEMU_OPTION_no_kvm_pit: { >> + case QEMU_OPTION_no_kvm_pit: >> fprintf(stderr, "Warning: KVM PIT can no longer be disabled " >> "separately.\n"); >> break; >> - } >> case QEMU_OPTION_no_kvm_pit_reinjection: { >> static GlobalProperty kvm_pit_lost_tick_policy[] = { >> { >> @@ -3681,11 +3675,11 @@ int main(int argc, char **argv, char **envp) >> #ifdef CONFIG_VNC >> display_remote++; >> vnc_display = optarg; >> + break; >> #else >> fprintf(stderr, "VNC support is disabled\n"); >> exit(1); >> #endif >> - break; >> case QEMU_OPTION_no_acpi: >> acpi_enabled = 0; >> break; >> @@ -3811,7 +3805,6 @@ int main(int argc, char **argv, char **envp) >> xen_mode = XEN_ATTACH; >> break; >> case QEMU_OPTION_trace: >> - { >> opts = qemu_opts_parse(qemu_find_opts("trace"), optarg, 0); >> if (!opts) { >> exit(1); >> @@ -3819,7 +3812,6 @@ int main(int argc, char **argv, char **envp) >> trace_events = qemu_opt_get(opts, "events"); >> trace_file = qemu_opt_get(opts, "file"); >> break; >> - } >> case QEMU_OPTION_readconfig: >> { >> int ret = qemu_read_config_file(optarg); >> @@ -3876,12 +3868,12 @@ int main(int argc, char **argv, char **envp) >> if (!opts) { >> exit(1); >> } >> + break; >> #else >> error_report("File descriptor passing is disabled on this " >> "platform"); >> exit(1); >> #endif >> - break; >> case QEMU_OPTION_object: >> opts = qemu_opts_parse(qemu_find_opts("object"), optarg, 1); >> if (!opts) { >> @@ -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(¤t_machine->init_args); >> >> audio_init(); >>
On 04/01/2014 08:36 PM, Alex Bennée wrote: > > Chen Gang <gang.chen.5i5j@gmail.com> writes: > >> Hello Maintainers: >> >> In main switch of main(), it contents several styles for "{...}" code block. >> >> If it is necessary to use unique style within a function, please let me >> know, I will/should clean up it. And also better to tell me which style >> we need choose -- for me, I don't know which style is the best to >> Qemu. > > The correct block style is described in CODING_STYLE Section 4. However > chunks of the code base violate the style guidelines and should be > cleaned up as other fixes are made. > In CODING_STYLE Section 4, has no demo for "{...}" within switch, I guess your meaning is "the demo below is the correct block style for Qemu": switch(...) { case 'x': { char a; ... break; } case 'y': ... break; default: break; } If it is OK, suggest to complete the CODING_STYLE Section 4 with one 'switch' demo. Thanks.
Chen Gang <gang.chen.5i5j@gmail.com> writes: > On 04/01/2014 04:13 PM, Markus Armbruster wrote: >> Chen Gang <gang.chen.5i5j@gmail.com> writes: >> >>> On 03/31/2014 11:49 PM, Markus Armbruster wrote: >>>> Chen Gang <gang.chen.5i5j@gmail.com> 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). >> >> No rush :) >> > > Yeah, more discussions, and more thinking of, before implementations. What I meant to say is "next week is fine, we're not in a big hurry to get this done".
On 04/01/2014 09:33 PM, Markus Armbruster wrote: > Chen Gang <gang.chen.5i5j@gmail.com> writes: > >> On 04/01/2014 04:13 PM, Markus Armbruster wrote: >>> Chen Gang <gang.chen.5i5j@gmail.com> writes: >>> >>>> On 03/31/2014 11:49 PM, Markus Armbruster wrote: >>>>> Chen Gang <gang.chen.5i5j@gmail.com> 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). >>> >>> No rush :) >>> >> >> Yeah, more discussions, and more thinking of, before implementations. > > What I meant to say is "next week is fine, we're not in a big hurry to > get this done". > OK, thanks. :-) Thanks.
diff --git a/vl.c b/vl.c index 9975e5a..9c733cb 100644 --- a/vl.c +++ b/vl.c @@ -1188,18 +1188,16 @@ DeviceState *get_boot_device(uint32_t position) { uint32_t counter = 0; FWBootEntry *i = NULL; - DeviceState *res = NULL; if (!QTAILQ_EMPTY(&fw_boot_order)) { QTAILQ_FOREACH(i, &fw_boot_order, link) { if (counter == position) { - res = i->dev; - break; + return i->dev; } counter++; } } - return res; + return NULL; } /* @@ -3034,7 +3032,6 @@ int main(int argc, char **argv, char **envp) if (argv[optind][0] != '-') { /* disk image */ optind++; - continue; } else { const QEMUOption *popt; @@ -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; @@ -3286,7 +3283,6 @@ int main(int argc, char **argv, char **envp) case QEMU_OPTION_audio_help: AUD_help (); exit (0); - break; case QEMU_OPTION_soundhw: select_soundhw (optarg); break; @@ -3296,7 +3292,6 @@ int main(int argc, char **argv, char **envp) case QEMU_OPTION_version: version(); exit(0); - break; case QEMU_OPTION_m: { int64_t value; uint64_t sz; @@ -3638,11 +3633,10 @@ int main(int argc, char **argv, char **envp) olist = qemu_find_opts("machine"); qemu_opts_parse(olist, "accel=tcg", 0); break; - case QEMU_OPTION_no_kvm_pit: { + case QEMU_OPTION_no_kvm_pit: fprintf(stderr, "Warning: KVM PIT can no longer be disabled " "separately.\n"); break; - } case QEMU_OPTION_no_kvm_pit_reinjection: { static GlobalProperty kvm_pit_lost_tick_policy[] = { { @@ -3681,11 +3675,11 @@ int main(int argc, char **argv, char **envp) #ifdef CONFIG_VNC display_remote++; vnc_display = optarg; + break; #else fprintf(stderr, "VNC support is disabled\n"); exit(1); #endif - break; case QEMU_OPTION_no_acpi: acpi_enabled = 0; break; @@ -3811,7 +3805,6 @@ int main(int argc, char **argv, char **envp) xen_mode = XEN_ATTACH; break; case QEMU_OPTION_trace: - { opts = qemu_opts_parse(qemu_find_opts("trace"), optarg, 0); if (!opts) { exit(1); @@ -3819,7 +3812,6 @@ int main(int argc, char **argv, char **envp) trace_events = qemu_opt_get(opts, "events"); trace_file = qemu_opt_get(opts, "file"); break; - } case QEMU_OPTION_readconfig: { int ret = qemu_read_config_file(optarg); @@ -3876,12 +3868,12 @@ int main(int argc, char **argv, char **envp) if (!opts) { exit(1); } + break; #else error_report("File descriptor passing is disabled on this " "platform"); exit(1); #endif - break; case QEMU_OPTION_object: opts = qemu_opts_parse(qemu_find_opts("object"), optarg, 1); if (!opts) { @@ -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(¤t_machine->init_args); audio_init();
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). Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> --- vl.c | 36 +++++++++++++----------------------- 1 file changed, 13 insertions(+), 23 deletions(-)