Re: [PATCH for-4.2] hw/i386: Fix compiler warning when CONFIG_IDE_ISA is disabled

[Philippe Mathieu-Daudé]

On 11/15/19 5:12 PM, Thomas Huth wrote:
> On 15/11/2019 17.15, Peter Maydell wrote:
> > On Fri, 15 Nov 2019 at 16:08, Thomas Huth <address@hidden> wrote:
> > > On 15/11/2019 16.54, Peter Maydell wrote:
> > > > On Fri, 15 Nov 2019 at 15:10, Thomas Huth <address@hidden> wrote:
> > > > > --- a/hw/i386/pc_piix.c
> > > > > +++ b/hw/i386/pc_piix.c
> > > > > @@ -78,7 +78,6 @@ static void pc_init1(MachineState *machine,
> > > > >       X86MachineState *x86ms = X86_MACHINE(machine);
> > > > >       MemoryRegion *system_memory = get_system_memory();
> > > > >       MemoryRegion *system_io = get_system_io();
> > > > > -    int i;
> > > > >       PCIBus *pci_bus;
> > > > >       ISABus *isa_bus;
> > > > >       PCII440FXState *i440fx_state;
> > > > > @@ -253,7 +252,7 @@ static void pc_init1(MachineState *machine,
> > > > >       }
> > > > >   #ifdef CONFIG_IDE_ISA
> > > > >   else {
> > > > > -        for(i = 0; i < MAX_IDE_BUS; i++) {
> > > > > +        for (int i = 0; i < MAX_IDE_BUS; i++) {
> > > > >               ISADevice *dev;
> > > > >               char busname[] = "ide.0";
> > > > >               dev = isa_ide_init(isa_bus, ide_iobase[i], ide_iobase2[i],
> > > >
> > > > Don't put variable declarations inside 'for' statements,
> > > > please. They should go at the start of a {} block.
> > >
> > > Why? We're using -std=gnu99 now, so this should not be an issue anymore.
> >
> > Consistency with the rest of the code base, which mostly
> > avoids this particular trick.
>
> We've also got a few spots that use it...
> (run e.g.: grep -r 'for (int ' hw/* )

~31 (vs 8000+):

$ git grep -E 'for\s*\((int|size_t)'|egrep -v '\.(cc|cpp):'
audio/audio_legacy.c:400:        for (int i = 0; audio_prio_list[i]; i++) {
hw/block/pflash_cfi02.c:281:    for (int i = 0; i < 
pflash_regions_count(pfl); ++i) {
hw/block/pflash_cfi02.c:889:    for (int i = 0; i < nb_regions; ++i) {
hw/i386/fw_cfg.c:39:    for (size_t i = 0; i < 
ARRAY_SIZE(fw_cfg_arch_wellknown_keys); i++) {
hw/i386/pc.c:1478:    for (size_t i = 0; i < ISA_NUM_IRQS; i++) {
hw/isa/lpc_ich9.c:70:    for (intx = 0; intx < PCI_NUM_PINS; intx++) {
hw/isa/lpc_ich9.c:126:        for (intx = 0; intx < PCI_NUM_PINS; intx++) {
hw/microblaze/xlnx-zynqmp-pmu.c:71:    for (int i = 0; i < 
XLNX_ZYNQMP_PMU_NUM_IPIS; i++) {
hw/microblaze/xlnx-zynqmp-pmu.c:124:    for (int i = 0; i < 
XLNX_ZYNQMP_PMU_NUM_IPIS; i++) {
hw/mips/cisco_c3600.c:472:    for (int i = 0; i < ISA_NUM_IRQS; i++) {
hw/mips/mips_malta.c:1471:    for (int i = 0; i < ISA_NUM_IRQS; i++) {
hw/ppc/fw_cfg.c:39:    for (size_t i = 0; i < 
ARRAY_SIZE(fw_cfg_arch_wellknown_keys); i++) {
hw/riscv/sifive_e.c:187:    for (int i = 0; i < 32; i++) {
hw/riscv/sifive_gpio.c:32:    for (int i = 0; i < SIFIVE_GPIO_PINS; i++) {
hw/riscv/sifive_gpio.c:360:    for (int i = 0; i < SIFIVE_GPIO_PINS; i++) {
hw/sd/aspeed_sdhci.c:130:    for (int i = 0; i < ASPEED_SDHCI_NUM_SLOTS; 
++i) {
hw/sparc/sun4m.c:117:    for (size_t i = 0; i < 
ARRAY_SIZE(fw_cfg_arch_wellknown_keys); i++) {
hw/sparc64/sun4u.c:108:    for (size_t i = 0; i < 
ARRAY_SIZE(fw_cfg_arch_wellknown_keys); i++) {
hw/usb/hcd-xhci.c:3553:    for (intr = 0; intr < xhci->numintrs; intr++) {
hw/virtio/vhost.c:454:        for (int i = 0; i < n_old_sections; i++) {
qemu-nbd.c:302:            for (size_t bit = 0; bit < 
ARRAY_SIZE(flag_names); bit++) {
target/i386/hvf/hvf.c:497:    for (int i = 0; i < 8; i++) {
target/i386/hvf/x86_decode.c:34:    for (int i = 0; i < 
decode->opcode_len; i++) {
tests/pflash-cfi02-test.c:343:    for (int region = 0; region < 
nb_erase_regions; ++region) {
tests/pflash-cfi02-test.c:407:    for (int region = 0; region < 
nb_erase_regions; ++region) {
tests/pflash-cfi02-test.c:447:    for (int region = 0; region < 
nb_erase_regions; ++region) {
tests/pflash-cfi02-test.c:448:        for (int i = 0; i < 
config->nb_blocs[region]; ++i) {
tests/pflash-cfi02-test.c:458:    for (int region = 0; region < 
nb_erase_regions; ++region) {
tests/pflash-cfi02-test.c:469:    for (int region = 0; region < 
nb_erase_regions; ++region) {
tests/pflash-cfi02-test.c:470:        for (int i = 0; i < 
config->nb_blocs[region]; ++i) {
tests/pflash-cfi02-test.c:658:    for (size_t i = 0; i < 
nb_configurations; ++i) {

[I introduced most of them without respecting the CODING_STYLE,
 but checkpatch didn't complained].

> > See the 'Declarations' section of CODING_STYLE.rst.
>
> OK, that's a point. But since this gnu99 is a rather new option that we
> just introduced less than a year ago, we should maybe think of whether
> we want to allow this for for-loops now, too (since IMHO it's quite a
> nice feature in gnu99).

I agree with Thomas. I started to clean some signed/unsigned warnings 
and various cases the scope of some variables is confuse.

The related question is, is it OK to use size_t to iterate over an array?

  for (size_t i = 0; i < ARRAY_SIZE(array); i++) {
    ...
  }

Asking in this thread so we can modify CODING_STYLE accordingly :)