Horton

Re: shadow variables - pg15 edition

Peter Smith <smithpb2250@gmail.com>

View thread View on postgr.es
From: Peter Smith <smithpb2250@gmail.com>
To: David Rowley <dgrowleyml@gmail.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>, Tomas Vondra <tomas.vondra@postgresql.org>
Date: 2022-08-18T08:26:33Z
Lists: pgsql-hackers 
On Thu, Aug 18, 2022 at 5:27 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Thu, 18 Aug 2022 at 17:16, Justin Pryzby <pryzby@telsasoft.com> wrote:
> >
> > On Thu, Aug 18, 2022 at 03:17:33PM +1200, David Rowley wrote:
> > > I'm probably not the only committer to want to run a mile when they
> > > see someone posting 17 or 26 patches in an email. So maybe "bang for
> > > buck" is a better method for getting the ball rolling here.  As you
> > > know, I was recently bitten by local shadows in af7d270dd, so I do
> > > believe in the cause.
> > >
> > > What do you think?
> >
> > You already fixed the shadow var introduced in master/pg16, and I sent patches
> > for the shadow vars added in pg15 (marked as such and presented as 001-008), so
> > perhaps it's okay to start with that ?
>
> Alright,  I made a pass over the 0001-0008 patches.
>
...

>
> 0005. How about just "tslot". I'm not a fan of "this".
>

(I'm sure there are others like this; I just picked this one as an example)

AFAICT the offending 'slot' really should have never been declared at
all at the local scope in the first place - e.g. the other code in
this function seems happy enough with the pattern of just re-using the
function scoped 'slot'.

I understand that for this shadow patch changing the var-name is
considered the saner/safer way than tampering with the scope, but
perhaps it is still useful to include a comment when changing ones
like this?

e.g.
+ TupleTableSlot *tslot; /* TODO - Why declare this at all? Shouldn't
it just re-use the 'slot' at function scope? */

Otherwise, such knowledge will be lost, and nobody will ever know to
revisit them, which feels a bit more like *hiding* the mistake than
fixing it.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

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