Re: [PATCH 1/1] meson: Enable -Wshadow=local

[Daniel P . Berrangé]

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 :|