* Re: [PATCH 1/1] meson: Enable -Wshadow=local
2023-10-26 5:31 ` [PATCH 1/1] meson: " Markus Armbruster
@ 2023-10-26 5:44 ` Thomas Huth
2023-10-26 5:51 ` Warner Losh
2023-10-26 5:58 ` Philippe Mathieu-Daudé
2 siblings, 0 replies; 15+ messages in thread
From: Thomas Huth @ 2023-10-26 5:44 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: pbonzini, marcandre.lureau, berrange, philmd, bcain, imp,
stefanha
On 26/10/2023 07.31, Markus Armbruster wrote:
> Local variables shadowing other local variables or parameters make the
> code needlessly hard to understand. Bugs love to hide in such code.
> Evidence: commit bbde656263d (migration/rdma: Fix save_page method to
> fail on polling error).
>
> Enable -Wshadow=local to prevent such issues. Possible thanks to
> recent cleanups. Enabling -Wshadow would prevent more issues, but
> we're not yet ready for that.
>
> As usual, the warning is only enabled when the compiler recognizes it.
> GCC does, Clang doesn't.
>
> Some shadowed locals remain in bsd-user. Since BSD prefers Clang,
> let's not wait for its cleanup.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> meson.build | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/meson.build b/meson.build
> index dcef8b1e79..89220443b8 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -462,6 +462,7 @@ warn_flags = [
> '-Wno-tautological-type-limit-compare',
> '-Wno-psabi',
> '-Wno-gnu-variable-sized-type-not-at-end',
> + '-Wshadow=local',
> ]
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] meson: Enable -Wshadow=local
2023-10-26 5:31 ` [PATCH 1/1] meson: " Markus Armbruster
2023-10-26 5:44 ` Thomas Huth
@ 2023-10-26 5:51 ` Warner Losh
2023-10-26 5:55 ` Thomas Huth
2023-10-26 5:58 ` Philippe Mathieu-Daudé
2 siblings, 1 reply; 15+ messages in thread
From: Warner Losh @ 2023-10-26 5:51 UTC (permalink / raw)
To: Markus Armbruster
Cc: QEMU Developers, Paolo Bonzini, Marc-André Lureau,
Daniel P. Berrange, Thomas Huth, Philippe Mathieu-Daudé,
Brian Cain, Stefan Hajnoczi
[-- Attachment #1: Type: text/plain, Size: 1233 bytes --]
On Wed, Oct 25, 2023, 11:31 PM Markus Armbruster <armbru@redhat.com> wrote:
> Local variables shadowing other local variables or parameters make the
> code needlessly hard to understand. Bugs love to hide in such code.
> Evidence: commit bbde656263d (migration/rdma: Fix save_page method to
> fail on polling error).
>
> Enable -Wshadow=local to prevent such issues. Possible thanks to
> recent cleanups. Enabling -Wshadow would prevent more issues, but
> we're not yet ready for that.
>
> As usual, the warning is only enabled when the compiler recognizes it.
> GCC does, Clang doesn't.
>
> Some shadowed locals remain in bsd-user. Since BSD prefers Clang,
> let's not wait for its cleanup.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> meson.build | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/meson.build b/meson.build
> index dcef8b1e79..89220443b8 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -462,6 +462,7 @@ warn_flags = [
> '-Wno-tautological-type-limit-compare',
> '-Wno-psabi',
> '-Wno-gnu-variable-sized-type-not-at-end',
> + '-Wshadow=local',
>
Does this work with clang? I've not had good luck enabling it.
Warner
]
>
> if targetos != 'darwin'
> --
> 2.41.0
>
>
[-- Attachment #2: Type: text/html, Size: 2056 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] meson: Enable -Wshadow=local
2023-10-26 5:51 ` Warner Losh
@ 2023-10-26 5:55 ` Thomas Huth
0 siblings, 0 replies; 15+ messages in thread
From: Thomas Huth @ 2023-10-26 5:55 UTC (permalink / raw)
To: Warner Losh, Markus Armbruster
Cc: QEMU Developers, Paolo Bonzini, Marc-André Lureau,
Daniel P. Berrange, Philippe Mathieu-Daudé, Brian Cain,
Stefan Hajnoczi
On 26/10/2023 07.51, Warner Losh wrote:
>
>
> On Wed, Oct 25, 2023, 11:31 PM Markus Armbruster <armbru@redhat.com
> <mailto:armbru@redhat.com>> wrote:
>
> Local variables shadowing other local variables or parameters make the
> code needlessly hard to understand. Bugs love to hide in such code.
> Evidence: commit bbde656263d (migration/rdma: Fix save_page method to
> fail on polling error).
>
> Enable -Wshadow=local to prevent such issues. Possible thanks to
> recent cleanups. Enabling -Wshadow would prevent more issues, but
> we're not yet ready for that.
>
> As usual, the warning is only enabled when the compiler recognizes it.
> GCC does, Clang doesn't.
>
> Some shadowed locals remain in bsd-user. Since BSD prefers Clang,
> let's not wait for its cleanup.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com
> <mailto:armbru@redhat.com>>
> ---
> meson.build | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/meson.build b/meson.build
> index dcef8b1e79..89220443b8 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -462,6 +462,7 @@ warn_flags = [
> '-Wno-tautological-type-limit-compare',
> '-Wno-psabi',
> '-Wno-gnu-variable-sized-type-not-at-end',
> + '-Wshadow=local',
>
>
> Does this work with clang? I've not had good luck enabling it.
The flags are added via cc.get_supported_arguments(warn_flags), so meson
checks whether the compiler supports them before blindly adding them to the
list.
That means it should get ignored with Clang, i.e. we should be ok for the
remaining spots in the bsd-user code, assuming that most FreeBSD users will
use Clang to compile QEMU.
Thomas
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] meson: Enable -Wshadow=local
2023-10-26 5:31 ` [PATCH 1/1] meson: " Markus Armbruster
2023-10-26 5:44 ` Thomas Huth
2023-10-26 5:51 ` Warner Losh
@ 2023-10-26 5:58 ` Philippe Mathieu-Daudé
2023-10-26 6:12 ` Thomas Huth
2023-10-26 10:05 ` Daniel P. Berrangé
2 siblings, 2 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-10-26 5:58 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: pbonzini, marcandre.lureau, berrange, thuth, bcain, imp, stefanha,
Peter Maydell
On 26/10/23 07:31, Markus Armbruster wrote:
> Local variables shadowing other local variables or parameters make the
> code needlessly hard to understand. Bugs love to hide in such code.
> Evidence: commit bbde656263d (migration/rdma: Fix save_page method to
> fail on polling error).
>
> Enable -Wshadow=local to prevent such issues. Possible thanks to
> recent cleanups. Enabling -Wshadow would prevent more issues, but
> we're not yet ready for that.
>
> As usual, the warning is only enabled when the compiler recognizes it.
> GCC does, Clang doesn't.
>
> Some shadowed locals remain in bsd-user. Since BSD prefers Clang,
> let's not wait for its cleanup.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> meson.build | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/meson.build b/meson.build
> index dcef8b1e79..89220443b8 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -462,6 +462,7 @@ warn_flags = [
> '-Wno-tautological-type-limit-compare',
> '-Wno-psabi',
> '-Wno-gnu-variable-sized-type-not-at-end',
> + '-Wshadow=local',
> ]
>
> if targetos != 'darwin'
Using Clang on Darwin:
$ ../configure
The Meson build system
Version: 1.2.1
Build type: native build
Project name: qemu
Project version: 8.1.50
C compiler for the host machine: cc (clang 15.0.0 "Apple clang version
15.0.0 (clang-1500.0.40.1)")
C linker for the host machine: cc ld64 1015.7
Host machine cpu family: aarch64
Host machine cpu: aarch64
Program sh found: YES (/bin/sh)
Objective-C compiler for the host machine: clang (clang 15.0.0)
Objective-C linker for the host machine: clang ld64 1015.7
Program bzip2 found: YES (/usr/bin/bzip2)
Program iasl found: YES (/opt/homebrew/bin/iasl)
Compiler for C supports arguments -fno-pie: YES
Compiler for C supports arguments -no-pie: NO
Compiler for C supports link arguments -Wl,-z,relro: NO
Compiler for C supports link arguments -Wl,-z,now: NO
Compiler for C supports link arguments -Wl,--warn-common: NO
Compiler for C supports arguments -Wundef: YES
Compiler for C supports arguments -Wwrite-strings: YES
Compiler for C supports arguments -Wmissing-prototypes: YES
Compiler for C supports arguments -Wstrict-prototypes: YES
Compiler for C supports arguments -Wredundant-decls: YES
Compiler for C supports arguments -Wold-style-declaration: NO
Compiler for C supports arguments -Wold-style-definition: YES
Compiler for C supports arguments -Wtype-limits: YES
Compiler for C supports arguments -Wformat-security: YES
Compiler for C supports arguments -Wformat-y2k: YES
Compiler for C supports arguments -Winit-self: YES
Compiler for C supports arguments -Wignored-qualifiers: YES
Compiler for C supports arguments -Wempty-body: YES
Compiler for C supports arguments -Wnested-externs: YES
Compiler for C supports arguments -Wendif-labels: YES
Compiler for C supports arguments -Wexpansion-to-defined: YES
Compiler for C supports arguments -Wimplicit-fallthrough=2: NO
Compiler for C supports arguments -Wmissing-format-attribute: YES
Compiler for C supports arguments -Wno-initializer-overrides: YES
Compiler for C supports arguments -Wno-missing-include-dirs: YES
Compiler for C supports arguments -Wno-shift-negative-value: YES
Compiler for C supports arguments -Wno-string-plus-int: YES
Compiler for C supports arguments -Wno-typedef-redefinition: YES
Compiler for C supports arguments -Wno-tautological-type-limit-compare: YES
Compiler for C supports arguments -Wno-psabi: YES
Compiler for C supports arguments
-Wno-gnu-variable-sized-type-not-at-end: YES
Compiler for C supports arguments -Wshadow=local: NO
Compiler for Objective-C supports arguments -Wundef: YES
Compiler for Objective-C supports arguments -Wwrite-strings: YES
Compiler for Objective-C supports arguments -Wmissing-prototypes: YES
Compiler for Objective-C supports arguments -Wstrict-prototypes: YES
Compiler for Objective-C supports arguments -Wredundant-decls: YES
Compiler for Objective-C supports arguments -Wold-style-declaration: NO
Compiler for Objective-C supports arguments -Wold-style-definition: YES
Compiler for Objective-C supports arguments -Wtype-limits: YES
Compiler for Objective-C supports arguments -Wformat-security: YES
Compiler for Objective-C supports arguments -Wformat-y2k: YES
Compiler for Objective-C supports arguments -Winit-self: YES
Compiler for Objective-C supports arguments -Wignored-qualifiers: YES
Compiler for Objective-C supports arguments -Wempty-body: YES
Compiler for Objective-C supports arguments -Wnested-externs: YES
Compiler for Objective-C supports arguments -Wendif-labels: YES
Compiler for Objective-C supports arguments -Wexpansion-to-defined: YES
Compiler for Objective-C supports arguments -Wimplicit-fallthrough=2: NO
Compiler for Objective-C supports arguments -Wmissing-format-attribute: YES
Compiler for Objective-C supports arguments -Wno-initializer-overrides: YES
Compiler for Objective-C supports arguments -Wno-missing-include-dirs: YES
Compiler for Objective-C supports arguments -Wno-shift-negative-value: YES
Compiler for Objective-C supports arguments -Wno-string-plus-int: YES
Compiler for Objective-C supports arguments -Wno-typedef-redefinition: YES
Compiler for Objective-C supports arguments
-Wno-tautological-type-limit-compare: YES
Compiler for Objective-C supports arguments -Wno-psabi: YES
Compiler for Objective-C supports arguments
-Wno-gnu-variable-sized-type-not-at-end: YES
Compiler for Objective-C supports arguments -Wshadow=local: NO
So:
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Now don't blame me for posting patches with trigger shadow=local
warnings because I am not testing that locally.
I find it a bit unfair to force me rely on CI or other machines
rather than my host machine to check for warnings. I'd have
rather waited this option support lands first in Clang before
enabling this flag.
Regards,
Phil.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] meson: Enable -Wshadow=local
2023-10-26 5:58 ` Philippe Mathieu-Daudé
@ 2023-10-26 6:12 ` Thomas Huth
2023-10-26 6:17 ` Philippe Mathieu-Daudé
2023-10-26 10:05 ` Daniel P. Berrangé
1 sibling, 1 reply; 15+ messages in thread
From: Thomas Huth @ 2023-10-26 6:12 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Markus Armbruster, qemu-devel
Cc: pbonzini, marcandre.lureau, berrange, bcain, imp, stefanha,
Peter Maydell
On 26/10/2023 07.58, Philippe Mathieu-Daudé wrote:
> On 26/10/23 07:31, Markus Armbruster wrote:
>> Local variables shadowing other local variables or parameters make the
>> code needlessly hard to understand. Bugs love to hide in such code.
>> Evidence: commit bbde656263d (migration/rdma: Fix save_page method to
>> fail on polling error).
>>
>> Enable -Wshadow=local to prevent such issues. Possible thanks to
>> recent cleanups. Enabling -Wshadow would prevent more issues, but
>> we're not yet ready for that.
>>
>> As usual, the warning is only enabled when the compiler recognizes it.
>> GCC does, Clang doesn't.
>>
>> Some shadowed locals remain in bsd-user. Since BSD prefers Clang,
>> let's not wait for its cleanup.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> meson.build | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/meson.build b/meson.build
>> index dcef8b1e79..89220443b8 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -462,6 +462,7 @@ warn_flags = [
>> '-Wno-tautological-type-limit-compare',
>> '-Wno-psabi',
>> '-Wno-gnu-variable-sized-type-not-at-end',
>> + '-Wshadow=local',
>> ]
>> if targetos != 'darwin'
>
> Using Clang on Darwin:
>
> $ ../configure
> The Meson build system
> Version: 1.2.1
> Build type: native build
> Project name: qemu
> Project version: 8.1.50
> C compiler for the host machine: cc (clang 15.0.0 "Apple clang version
> 15.0.0 (clang-1500.0.40.1)")
> C linker for the host machine: cc ld64 1015.7
> Host machine cpu family: aarch64
> Host machine cpu: aarch64
> Program sh found: YES (/bin/sh)
> Objective-C compiler for the host machine: clang (clang 15.0.0)
> Objective-C linker for the host machine: clang ld64 1015.7
> Program bzip2 found: YES (/usr/bin/bzip2)
> Program iasl found: YES (/opt/homebrew/bin/iasl)
> Compiler for C supports arguments -fno-pie: YES
> Compiler for C supports arguments -no-pie: NO
> Compiler for C supports link arguments -Wl,-z,relro: NO
> Compiler for C supports link arguments -Wl,-z,now: NO
> Compiler for C supports link arguments -Wl,--warn-common: NO
> Compiler for C supports arguments -Wundef: YES
> Compiler for C supports arguments -Wwrite-strings: YES
> Compiler for C supports arguments -Wmissing-prototypes: YES
> Compiler for C supports arguments -Wstrict-prototypes: YES
> Compiler for C supports arguments -Wredundant-decls: YES
> Compiler for C supports arguments -Wold-style-declaration: NO
> Compiler for C supports arguments -Wold-style-definition: YES
> Compiler for C supports arguments -Wtype-limits: YES
> Compiler for C supports arguments -Wformat-security: YES
> Compiler for C supports arguments -Wformat-y2k: YES
> Compiler for C supports arguments -Winit-self: YES
> Compiler for C supports arguments -Wignored-qualifiers: YES
> Compiler for C supports arguments -Wempty-body: YES
> Compiler for C supports arguments -Wnested-externs: YES
> Compiler for C supports arguments -Wendif-labels: YES
> Compiler for C supports arguments -Wexpansion-to-defined: YES
> Compiler for C supports arguments -Wimplicit-fallthrough=2: NO
> Compiler for C supports arguments -Wmissing-format-attribute: YES
> Compiler for C supports arguments -Wno-initializer-overrides: YES
> Compiler for C supports arguments -Wno-missing-include-dirs: YES
> Compiler for C supports arguments -Wno-shift-negative-value: YES
> Compiler for C supports arguments -Wno-string-plus-int: YES
> Compiler for C supports arguments -Wno-typedef-redefinition: YES
> Compiler for C supports arguments -Wno-tautological-type-limit-compare: YES
> Compiler for C supports arguments -Wno-psabi: YES
> Compiler for C supports arguments -Wno-gnu-variable-sized-type-not-at-end: YES
> Compiler for C supports arguments -Wshadow=local: NO
> Compiler for Objective-C supports arguments -Wundef: YES
> Compiler for Objective-C supports arguments -Wwrite-strings: YES
> Compiler for Objective-C supports arguments -Wmissing-prototypes: YES
> Compiler for Objective-C supports arguments -Wstrict-prototypes: YES
> Compiler for Objective-C supports arguments -Wredundant-decls: YES
> Compiler for Objective-C supports arguments -Wold-style-declaration: NO
> Compiler for Objective-C supports arguments -Wold-style-definition: YES
> Compiler for Objective-C supports arguments -Wtype-limits: YES
> Compiler for Objective-C supports arguments -Wformat-security: YES
> Compiler for Objective-C supports arguments -Wformat-y2k: YES
> Compiler for Objective-C supports arguments -Winit-self: YES
> Compiler for Objective-C supports arguments -Wignored-qualifiers: YES
> Compiler for Objective-C supports arguments -Wempty-body: YES
> Compiler for Objective-C supports arguments -Wnested-externs: YES
> Compiler for Objective-C supports arguments -Wendif-labels: YES
> Compiler for Objective-C supports arguments -Wexpansion-to-defined: YES
> Compiler for Objective-C supports arguments -Wimplicit-fallthrough=2: NO
> Compiler for Objective-C supports arguments -Wmissing-format-attribute: YES
> Compiler for Objective-C supports arguments -Wno-initializer-overrides: YES
> Compiler for Objective-C supports arguments -Wno-missing-include-dirs: YES
> Compiler for Objective-C supports arguments -Wno-shift-negative-value: YES
> Compiler for Objective-C supports arguments -Wno-string-plus-int: YES
> Compiler for Objective-C supports arguments -Wno-typedef-redefinition: YES
> Compiler for Objective-C supports arguments
> -Wno-tautological-type-limit-compare: YES
> Compiler for Objective-C supports arguments -Wno-psabi: YES
> Compiler for Objective-C supports arguments
> -Wno-gnu-variable-sized-type-not-at-end: YES
> Compiler for Objective-C supports arguments -Wshadow=local: NO
>
> So:
>
> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> Now don't blame me for posting patches with trigger shadow=local
> warnings because I am not testing that locally.
>
> I find it a bit unfair to force me rely on CI or other machines
> rather than my host machine to check for warnings. I'd have
> rather waited this option support lands first in Clang before
> enabling this flag.
Huh, that situation is already pre-existing, e.g. with
-Wimplicit-fallthrough=2 ... and if you're too afraid, you can always
install gcc via homebrew to check.
Thomas
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] meson: Enable -Wshadow=local
2023-10-26 6:12 ` Thomas Huth
@ 2023-10-26 6:17 ` Philippe Mathieu-Daudé
2023-10-26 6:50 ` Markus Armbruster
0 siblings, 1 reply; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-10-26 6:17 UTC (permalink / raw)
To: Thomas Huth, Markus Armbruster, qemu-devel
Cc: pbonzini, marcandre.lureau, berrange, bcain, imp, stefanha,
Peter Maydell
On 26/10/23 08:12, Thomas Huth wrote:
> On 26/10/2023 07.58, Philippe Mathieu-Daudé wrote:
>> On 26/10/23 07:31, Markus Armbruster wrote:
>>> Local variables shadowing other local variables or parameters make the
>>> code needlessly hard to understand. Bugs love to hide in such code.
>>> Evidence: commit bbde656263d (migration/rdma: Fix save_page method to
>>> fail on polling error).
>>>
>>> Enable -Wshadow=local to prevent such issues. Possible thanks to
>>> recent cleanups. Enabling -Wshadow would prevent more issues, but
>>> we're not yet ready for that.
>>>
>>> As usual, the warning is only enabled when the compiler recognizes it.
>>> GCC does, Clang doesn't.
>>>
>>> Some shadowed locals remain in bsd-user. Since BSD prefers Clang,
>>> let's not wait for its cleanup.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>> meson.build | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/meson.build b/meson.build
>>> index dcef8b1e79..89220443b8 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -462,6 +462,7 @@ warn_flags = [
>>> '-Wno-tautological-type-limit-compare',
>>> '-Wno-psabi',
>>> '-Wno-gnu-variable-sized-type-not-at-end',
>>> + '-Wshadow=local',
>>> ]
>>> if targetos != 'darwin'
>>
>> Using Clang on Darwin:
>>
>> $ ../configure
>> The Meson build system
>> Version: 1.2.1
>> Build type: native build
>> Project name: qemu
>> Project version: 8.1.50
>> C compiler for the host machine: cc (clang 15.0.0 "Apple clang version
>> 15.0.0 (clang-1500.0.40.1)")
>> C linker for the host machine: cc ld64 1015.7
>> Host machine cpu family: aarch64
>> Host machine cpu: aarch64
>> Program sh found: YES (/bin/sh)
>> Objective-C compiler for the host machine: clang (clang 15.0.0)
>> Objective-C linker for the host machine: clang ld64 1015.7
>> Compiler for Objective-C supports arguments -Wshadow=local: NO
>>
>> So:
>>
>> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>
>> Now don't blame me for posting patches with trigger shadow=local
>> warnings because I am not testing that locally.
>>
>> I find it a bit unfair to force me rely on CI or other machines
>> rather than my host machine to check for warnings. I'd have
>> rather waited this option support lands first in Clang before
>> enabling this flag.
>
> Huh, that situation is already pre-existing, e.g. with
> -Wimplicit-fallthrough=2 ... and if you're too afraid, you can always
> install gcc via homebrew to check.
OK, fine.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] meson: Enable -Wshadow=local
2023-10-26 6:17 ` Philippe Mathieu-Daudé
@ 2023-10-26 6:50 ` Markus Armbruster
0 siblings, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2023-10-26 6:50 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Thomas Huth, qemu-devel, pbonzini, marcandre.lureau, berrange,
bcain, imp, stefanha, Peter Maydell
Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> On 26/10/23 08:12, Thomas Huth wrote:
>> On 26/10/2023 07.58, Philippe Mathieu-Daudé wrote:
[...]
>>> $ ../configure
>>> The Meson build system
>>> Version: 1.2.1
>>> Build type: native build
>>> Project name: qemu
>>> Project version: 8.1.50
>>> C compiler for the host machine: cc (clang 15.0.0 "Apple clang version 15.0.0 (clang-1500.0.40.1)")
>>> C linker for the host machine: cc ld64 1015.7
>>> Host machine cpu family: aarch64
>>> Host machine cpu: aarch64
>>> Program sh found: YES (/bin/sh)
>>> Objective-C compiler for the host machine: clang (clang 15.0.0)
>>> Objective-C linker for the host machine: clang ld64 1015.7
>
>
>>> Compiler for Objective-C supports arguments -Wshadow=local: NO
>>>
>>> So:
>>>
>>> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Thanks!
>>> Now don't blame me for posting patches with trigger shadow=local
>>> warnings because I am not testing that locally.
>>>
>>> I find it a bit unfair to force me rely on CI or other machines
>>> rather than my host machine to check for warnings. I'd have
>>> rather waited this option support lands first in Clang before
>>> enabling this flag.
I'm not forcing anyone just yet, I'm merely posting a patch to solicit
feedback :)
PRO: It stops the backsliding. Thomas had to fix two new instances
already.
CON: Developers using only Clang may post patches that fail CI. We
don't know how annoying that will be in practice.
>> Huh, that situation is already pre-existing, e.g. with -Wimplicit-fallthrough=2 ... and if you're too afraid, you can always install gcc via homebrew to check.
>
> OK, fine.
I suggest to take the patch now, and if the CON turns out to outweigh
the PRO, revert it.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] meson: Enable -Wshadow=local
2023-10-26 5:58 ` Philippe Mathieu-Daudé
2023-10-26 6:12 ` Thomas Huth
@ 2023-10-26 10:05 ` Daniel P. Berrangé
1 sibling, 0 replies; 15+ messages in thread
From: Daniel P. Berrangé @ 2023-10-26 10:05 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Markus Armbruster, qemu-devel, pbonzini, marcandre.lureau, thuth,
bcain, imp, stefanha, Peter Maydell
On Thu, Oct 26, 2023 at 07:58:42AM +0200, Philippe Mathieu-Daudé wrote:
> On 26/10/23 07:31, Markus Armbruster wrote:
> > Local variables shadowing other local variables or parameters make the
> > code needlessly hard to understand. Bugs love to hide in such code.
> > Evidence: commit bbde656263d (migration/rdma: Fix save_page method to
> > fail on polling error).
> >
> > Enable -Wshadow=local to prevent such issues. Possible thanks to
> > recent cleanups. Enabling -Wshadow would prevent more issues, but
> > we're not yet ready for that.
> >
> > As usual, the warning is only enabled when the compiler recognizes it.
> > GCC does, Clang doesn't.
> >
> > Some shadowed locals remain in bsd-user. Since BSD prefers Clang,
> > let's not wait for its cleanup.
> >
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > ---
> > meson.build | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/meson.build b/meson.build
> > index dcef8b1e79..89220443b8 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -462,6 +462,7 @@ warn_flags = [
> > '-Wno-tautological-type-limit-compare',
> > '-Wno-psabi',
> > '-Wno-gnu-variable-sized-type-not-at-end',
> > + '-Wshadow=local',
> > ]
> > if targetos != 'darwin'
>
> Now don't blame me for posting patches with trigger shadow=local
> warnings because I am not testing that locally.
>
> I find it a bit unfair to force me rely on CI or other machines
> rather than my host machine to check for warnings. I'd have
> rather waited this option support lands first in Clang before
> enabling this flag.
QEMU has never required regular contributors to submit code that
compiles perfectly on every supported platform. Only that they
make a fair effort to have it compile on their platform, and
respond to feedback if a reviewer points out a problem for a
different platform.
Subsystem maintainers though should be ensuring code is warning
free on every platform by running through CI before submitting a
pull request.
This centralization of CI repsonsibilities on maintainers is one
of the downsides of our mailing list workflow, as compared to
gitforges where the regular contributors would immediately trigger
& see CI reports from every merge request they open.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 15+ messages in thread