Re: shadow variables - pg15 edition - Mailing list pgsql-hackers

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.