hw/i386/pc_piix.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
When CONFIG_IDE_ISA is disabled, compilation currently fails:
hw/i386/pc_piix.c: In function ‘pc_init1’:
hw/i386/pc_piix.c:81:9: error: unused variable ‘i’ [-Werror=unused-variable]
Move the variable declaration to the right code block to avoid
this problem.
Fixes: 4501d317b50e ("hw/i386/pc: Extract pc_i8259_create()")
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
hw/i386/pc_piix.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 2aefa3b8df..d187db761c 100644
--- 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],
--
2.23.0
On Fri, 15 Nov 2019 at 15:10, Thomas Huth <thuth@redhat.com> wrote: > > When CONFIG_IDE_ISA is disabled, compilation currently fails: > > hw/i386/pc_piix.c: In function ‘pc_init1’: > hw/i386/pc_piix.c:81:9: error: unused variable ‘i’ [-Werror=unused-variable] > > Move the variable declaration to the right code block to avoid > this problem. > > Fixes: 4501d317b50e ("hw/i386/pc: Extract pc_i8259_create()") > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > hw/i386/pc_piix.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 2aefa3b8df..d187db761c 100644 > --- 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. thanks -- PMM
On 15/11/2019 16.54, Peter Maydell wrote: > On Fri, 15 Nov 2019 at 15:10, Thomas Huth <thuth@redhat.com> wrote: >> >> When CONFIG_IDE_ISA is disabled, compilation currently fails: >> >> hw/i386/pc_piix.c: In function ‘pc_init1’: >> hw/i386/pc_piix.c:81:9: error: unused variable ‘i’ [-Werror=unused-variable] >> >> Move the variable declaration to the right code block to avoid >> this problem. >> >> Fixes: 4501d317b50e ("hw/i386/pc: Extract pc_i8259_create()") >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> hw/i386/pc_piix.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >> index 2aefa3b8df..d187db761c 100644 >> --- 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. Thomas
On 15/11/19 16:54, Thomas Huth wrote: > On 15/11/2019 16.54, Peter Maydell wrote: >> On Fri, 15 Nov 2019 at 15:10, Thomas Huth <thuth@redhat.com> wrote: >>> >>> When CONFIG_IDE_ISA is disabled, compilation currently fails: >>> >>> hw/i386/pc_piix.c: In function ‘pc_init1’: >>> hw/i386/pc_piix.c:81:9: error: unused variable ‘i’ [-Werror=unused-variable] >>> >>> Move the variable declaration to the right code block to avoid >>> this problem. >>> >>> Fixes: 4501d317b50e ("hw/i386/pc: Extract pc_i8259_create()") >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> hw/i386/pc_piix.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >>> index 2aefa3b8df..d187db761c 100644 >>> --- 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. For now I can squash the following while we discuss coding standards. :) diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index fa62244f4d..0130b8fb4e 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -244,7 +244,8 @@ static void pc_init1(MachineState *machine, } #ifdef CONFIG_IDE_ISA else { - for (int i = 0; i < MAX_IDE_BUS; i++) { + int i; + for (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], Paolo
On 15/11/2019 17.13, Paolo Bonzini wrote: > On 15/11/19 16:54, Thomas Huth wrote: >> On 15/11/2019 16.54, Peter Maydell wrote: >>> On Fri, 15 Nov 2019 at 15:10, Thomas Huth <thuth@redhat.com> wrote: >>>> >>>> When CONFIG_IDE_ISA is disabled, compilation currently fails: >>>> >>>> hw/i386/pc_piix.c: In function ‘pc_init1’: >>>> hw/i386/pc_piix.c:81:9: error: unused variable ‘i’ [-Werror=unused-variable] >>>> >>>> Move the variable declaration to the right code block to avoid >>>> this problem. >>>> >>>> Fixes: 4501d317b50e ("hw/i386/pc: Extract pc_i8259_create()") >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>> --- >>>> hw/i386/pc_piix.c | 3 +-- >>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>> >>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >>>> index 2aefa3b8df..d187db761c 100644 >>>> --- 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. > > For now I can squash the following while we discuss coding standards. :) > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index fa62244f4d..0130b8fb4e 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -244,7 +244,8 @@ static void pc_init1(MachineState *machine, > } > #ifdef CONFIG_IDE_ISA > else { > - for (int i = 0; i < MAX_IDE_BUS; i++) { > + int i; > + for (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], Yes, please do. I guess we won't update CODING_STYLE.rst during the hard freeze anymore ;-) Thomas
On 11/15/19 5:13 PM, Thomas Huth wrote: > On 15/11/2019 17.13, Paolo Bonzini wrote: >> On 15/11/19 16:54, Thomas Huth wrote: >>> On 15/11/2019 16.54, Peter Maydell wrote: >>>> On Fri, 15 Nov 2019 at 15:10, Thomas Huth <thuth@redhat.com> wrote: >>>>> >>>>> When CONFIG_IDE_ISA is disabled, compilation currently fails: >>>>> >>>>> hw/i386/pc_piix.c: In function ‘pc_init1’: >>>>> hw/i386/pc_piix.c:81:9: error: unused variable ‘i’ [-Werror=unused-variable] >>>>> >>>>> Move the variable declaration to the right code block to avoid >>>>> this problem. >>>>> >>>>> Fixes: 4501d317b50e ("hw/i386/pc: Extract pc_i8259_create()") >>>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>>> --- >>>>> hw/i386/pc_piix.c | 3 +-- >>>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>>> >>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >>>>> index 2aefa3b8df..d187db761c 100644 >>>>> --- 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. >> >> For now I can squash the following while we discuss coding standards. :) Thanks. >> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >> index fa62244f4d..0130b8fb4e 100644 >> --- a/hw/i386/pc_piix.c >> +++ b/hw/i386/pc_piix.c >> @@ -244,7 +244,8 @@ static void pc_init1(MachineState *machine, >> } >> #ifdef CONFIG_IDE_ISA >> else { >> - for (int i = 0; i < MAX_IDE_BUS; i++) { >> + int i; >> + for (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], > > Yes, please do. I guess we won't update CODING_STYLE.rst during the hard > freeze anymore ;-) Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On Fri, 15 Nov 2019 at 16:08, Thomas Huth <thuth@redhat.com> wrote: > > On 15/11/2019 16.54, Peter Maydell wrote: > > On Fri, 15 Nov 2019 at 15:10, Thomas Huth <thuth@redhat.com> 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. See the 'Declarations' section of CODING_STYLE.rst. As Paolo points out, there's a nice convenient block here already, so there's not much to be gained from putting the declaration in the middle of the for statement. thanks -- PMM
On 15/11/2019 17.15, Peter Maydell wrote: > On Fri, 15 Nov 2019 at 16:08, Thomas Huth <thuth@redhat.com> wrote: >> >> On 15/11/2019 16.54, Peter Maydell wrote: >>> On Fri, 15 Nov 2019 at 15:10, Thomas Huth <thuth@redhat.com> 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/* ) > 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). Thomas
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 <thuth@redhat.com> wrote: >>> >>> On 15/11/2019 16.54, Peter Maydell wrote: >>>> On Fri, 15 Nov 2019 at 15:10, Thomas Huth <thuth@redhat.com> 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 :)
Philippe Mathieu-Daudé <philmd@redhat.com> writes: [...] > 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++) { > ... > } My rule of thumb on integer types is "whatever lets me avoid not-obviously-safe conversions (implicit ones in particular) with the least type cast clutter. Quite often, int fits the bill. But not always. To reply to your example: depends on what's hiding in the ... :) > Asking in this thread so we can modify CODING_STYLE accordingly :)
On 18/11/2019 10.25, Markus Armbruster wrote: > Philippe Mathieu-Daudé <philmd@redhat.com> writes: > > [...] >> 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++) { >> ... >> } > > My rule of thumb on integer types is "whatever lets me avoid > not-obviously-safe conversions (implicit ones in particular) with the > least type cast clutter. > > Quite often, int fits the bill. But not always. > > To reply to your example: depends on what's hiding in the ... :) The problem here is that ARRAY_SIZE() gives you an size_t, so the compiler might complain about comparing signed int with unsigned size_t. Thus if i is only used as array index in the "..." part, I think it's fine to use size_t for i here. Thomas
On Fri, Nov 15, 2019 at 03:50:49PM +0100, Thomas Huth wrote: > When CONFIG_IDE_ISA is disabled, compilation currently fails: > > hw/i386/pc_piix.c: In function ‘pc_init1’: > hw/i386/pc_piix.c:81:9: error: unused variable ‘i’ [-Werror=unused-variable] > > Move the variable declaration to the right code block to avoid > this problem. > > Fixes: 4501d317b50e ("hw/i386/pc: Extract pc_i8259_create()") > Signed-off-by: Thomas Huth <thuth@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/i386/pc_piix.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 2aefa3b8df..d187db761c 100644 > --- 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], > -- > 2.23.0
© 2016 - 2024 Red Hat, Inc.