* [PATCH for-4.2] hw/i386: Fix compiler warning when CONFIG_IDE_ISA is disabled
@ 2019-11-15 14:50 Thomas Huth
2019-11-15 15:34 ` Michael S. Tsirkin
2019-11-15 15:54 ` Peter Maydell
0 siblings, 2 replies; 12+ messages in thread
From: Thomas Huth @ 2019-11-15 14:50 UTC (permalink / raw)
To: qemu-devel, Paolo Bonzini, Michael S. Tsirkin, Marcel Apfelbaum
Cc: qemu-trivial, Philippe Mathieu-Daudé, Eduardo Habkost
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH for-4.2] hw/i386: Fix compiler warning when CONFIG_IDE_ISA is disabled
2019-11-15 14:50 [PATCH for-4.2] hw/i386: Fix compiler warning when CONFIG_IDE_ISA is disabled Thomas Huth
@ 2019-11-15 15:34 ` Michael S. Tsirkin
2019-11-15 15:54 ` Peter Maydell
1 sibling, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2019-11-15 15:34 UTC (permalink / raw)
To: Thomas Huth
Cc: Eduardo Habkost, qemu-trivial, qemu-devel, Paolo Bonzini,
Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH for-4.2] hw/i386: Fix compiler warning when CONFIG_IDE_ISA is disabled
2019-11-15 14:50 [PATCH for-4.2] hw/i386: Fix compiler warning when CONFIG_IDE_ISA is disabled Thomas Huth
2019-11-15 15:34 ` Michael S. Tsirkin
@ 2019-11-15 15:54 ` Peter Maydell
2019-11-15 15:54 ` Thomas Huth
1 sibling, 1 reply; 12+ messages in thread
From: Peter Maydell @ 2019-11-15 15:54 UTC (permalink / raw)
To: Thomas Huth
Cc: Eduardo Habkost, Michael S. Tsirkin, QEMU Trivial,
QEMU Developers, Paolo Bonzini, Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH for-4.2] hw/i386: Fix compiler warning when CONFIG_IDE_ISA is disabled
2019-11-15 15:54 ` Peter Maydell
@ 2019-11-15 15:54 ` Thomas Huth
2019-11-15 16:13 ` Paolo Bonzini
2019-11-15 16:15 ` Peter Maydell
0 siblings, 2 replies; 12+ messages in thread
From: Thomas Huth @ 2019-11-15 15:54 UTC (permalink / raw)
To: Peter Maydell
Cc: Eduardo Habkost, Michael S. Tsirkin, QEMU Trivial,
QEMU Developers, Paolo Bonzini, Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH for-4.2] hw/i386: Fix compiler warning when CONFIG_IDE_ISA is disabled
2019-11-15 16:15 ` Peter Maydell
@ 2019-11-15 16:12 ` Thomas Huth
2019-11-15 20:31 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 12+ messages in thread
From: Thomas Huth @ 2019-11-15 16:12 UTC (permalink / raw)
To: Peter Maydell
Cc: Eduardo Habkost, Michael S. Tsirkin, QEMU Trivial,
QEMU Developers, Paolo Bonzini, Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH for-4.2] hw/i386: Fix compiler warning when CONFIG_IDE_ISA is disabled
2019-11-15 16:13 ` Paolo Bonzini
@ 2019-11-15 16:13 ` Thomas Huth
2019-11-15 20:33 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 12+ messages in thread
From: Thomas Huth @ 2019-11-15 16:13 UTC (permalink / raw)
To: Paolo Bonzini, Peter Maydell
Cc: Eduardo Habkost, Michael S. Tsirkin, QEMU Trivial,
QEMU Developers, Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH for-4.2] hw/i386: Fix compiler warning when CONFIG_IDE_ISA is disabled
2019-11-15 15:54 ` Thomas Huth
@ 2019-11-15 16:13 ` Paolo Bonzini
2019-11-15 16:13 ` Thomas Huth
2019-11-15 16:15 ` Peter Maydell
1 sibling, 1 reply; 12+ messages in thread
From: Paolo Bonzini @ 2019-11-15 16:13 UTC (permalink / raw)
To: Thomas Huth, Peter Maydell
Cc: Eduardo Habkost, Michael S. Tsirkin, QEMU Trivial,
QEMU Developers, Philippe Mathieu-Daudé
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH for-4.2] hw/i386: Fix compiler warning when CONFIG_IDE_ISA is disabled
2019-11-15 15:54 ` Thomas Huth
2019-11-15 16:13 ` Paolo Bonzini
@ 2019-11-15 16:15 ` Peter Maydell
2019-11-15 16:12 ` Thomas Huth
1 sibling, 1 reply; 12+ messages in thread
From: Peter Maydell @ 2019-11-15 16:15 UTC (permalink / raw)
To: Thomas Huth
Cc: Eduardo Habkost, Michael S. Tsirkin, QEMU Trivial,
QEMU Developers, Paolo Bonzini, Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH for-4.2] hw/i386: Fix compiler warning when CONFIG_IDE_ISA is disabled
2019-11-15 16:12 ` Thomas Huth
@ 2019-11-15 20:31 ` Philippe Mathieu-Daudé
2019-11-18 9:25 ` Markus Armbruster
0 siblings, 1 reply; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-11-15 20:31 UTC (permalink / raw)
To: Thomas Huth, Peter Maydell
Cc: Eduardo Habkost, Michael S. Tsirkin, QEMU Trivial,
QEMU Developers, Marc-André Lureau, Paolo Bonzini
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 :)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH for-4.2] hw/i386: Fix compiler warning when CONFIG_IDE_ISA is disabled
2019-11-15 16:13 ` Thomas Huth
@ 2019-11-15 20:33 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-11-15 20:33 UTC (permalink / raw)
To: Thomas Huth, Paolo Bonzini, Peter Maydell
Cc: QEMU Trivial, Eduardo Habkost, QEMU Developers, Michael S. Tsirkin
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>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH for-4.2] hw/i386: Fix compiler warning when CONFIG_IDE_ISA is disabled
2019-11-15 20:31 ` Philippe Mathieu-Daudé
@ 2019-11-18 9:25 ` Markus Armbruster
2019-11-18 9:28 ` Thomas Huth
0 siblings, 1 reply; 12+ messages in thread
From: Markus Armbruster @ 2019-11-18 9:25 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Thomas Huth, Eduardo Habkost, Michael S. Tsirkin,
QEMU Trivial, QEMU Developers, Paolo Bonzini,
Marc-André Lureau
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 :)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH for-4.2] hw/i386: Fix compiler warning when CONFIG_IDE_ISA is disabled
2019-11-18 9:25 ` Markus Armbruster
@ 2019-11-18 9:28 ` Thomas Huth
0 siblings, 0 replies; 12+ messages in thread
From: Thomas Huth @ 2019-11-18 9:28 UTC (permalink / raw)
To: Markus Armbruster, Philippe Mathieu-Daudé
Cc: Peter Maydell, Eduardo Habkost, Michael S. Tsirkin, QEMU Trivial,
QEMU Developers, Paolo Bonzini, Marc-André Lureau
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-11-18 9:30 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-15 14:50 [PATCH for-4.2] hw/i386: Fix compiler warning when CONFIG_IDE_ISA is disabled Thomas Huth
2019-11-15 15:34 ` Michael S. Tsirkin
2019-11-15 15:54 ` Peter Maydell
2019-11-15 15:54 ` Thomas Huth
2019-11-15 16:13 ` Paolo Bonzini
2019-11-15 16:13 ` Thomas Huth
2019-11-15 20:33 ` Philippe Mathieu-Daudé
2019-11-15 16:15 ` Peter Maydell
2019-11-15 16:12 ` Thomas Huth
2019-11-15 20:31 ` Philippe Mathieu-Daudé
2019-11-18 9:25 ` Markus Armbruster
2019-11-18 9:28 ` Thomas Huth
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.