Horton

Re: shadow variables - pg15 edition

Justin Pryzby <pryzby@telsasoft.com>

View thread View on postgr.es
From: Justin Pryzby <pryzby@telsasoft.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Peter Smith <smithpb2250@gmail.com>, pgsql-hackers@postgresql.org, Tomas Vondra <tomas.vondra@postgresql.org>, David Rowley <dgrowleyml@gmail.com>
Date: 2022-08-18T02:36:26Z
Lists: pgsql-hackers

Attachments

On Thu, Aug 18, 2022 at 09:39:02AM +0900, Michael Paquier wrote:
> On Thu, Aug 18, 2022 at 08:49:14AM +1000, Peter Smith wrote:
> > I'd started looking at these [1] last year and spent a day trying to
> > categorise them all in a spreadsheet (shadows a global, shadows a
> > parameter, shadows a local var etc) but I became swamped by the
> > volume, and then other work/life got in the way.
> > 
> > +1 from me.
> 
> A lot of the changes proposed here update the code so as the same
> variable gets used across more code paths by removing declarations,
> but we have two variables defined because both are aimed to be used in
> a different context (see AttachPartitionEnsureIndexes() in tablecmds.c
> for example).

> Wouldn't it be a saner approach in a lot of cases to rename the
> shadowed variables (aka the ones getting removed in your patches) and
> keep them local to the code paths where we use them?

The cases where I removed a declaration are ones where the variable either
hasn't yet been assigned in the outer scope (so it's safe to use first in the
inner scope, since its value is later overwriten in the outer scope).  Or it's
no longer used in the outer scope, so it's safe to re-use it in the inner scope
(as in AttachPartitionEnsureIndexes).  Since you think it's saner, I changed to
rename them.

In the case of "first", the var is used in two independent loops, the same way,
and re-initialized.  In the case of found_whole_row, the var is ignored, as the
comments say, so it would be silly to declare more vars to be additionally
ignored.

-- 
Justin

PS. I hadn't sent the other patches which rename the variables, having assumed
that the discussion would be bikeshedded to death and derail without having
fixed the lowest hanging fruits.  I'm attaching them those now to see what
happens.

Commits

  1. Fix shadow variable in postgres.c

  2. Add -Wshadow=compatible-local to the standard compilation flags

  3. Fix final warnings produced by -Wshadow=compatible-local

  4. Fix final compiler warning produced by -Wshadow=compatible-local

  5. Add optional parameter to PG_TRY() macros

  6. Small refactor to get rid of -Wshadow=compatible-local warning

  7. More -Wshadow=compatible-local warning fixes

  8. Further -Wshadow=compatible-local warning fixes

  9. Further reduce warnings with -Wshadow=compatible-local

  10. Reduce warnings with -Wshadow=compatible-local builds

  11. Remove shadowed local variables that are new in v15