Horton

Re: shadow variables - pg15 edition

Justin Pryzby <pryzby@telsasoft.com>

View thread View on postgr.es
From: Justin Pryzby <pryzby@telsasoft.com>
To: David Rowley <dgrowleyml@gmail.com>
Cc: pgsql-hackers@postgresql.org, Tomas Vondra <tomas.vondra@postgresql.org>, Peter Smith <smithpb2250@gmail.com>, Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: 2022-08-24T14:00:27Z
Lists: pgsql-hackers 
On Wed, Aug 24, 2022 at 10:47:31PM +1200, David Rowley wrote:
> I was hoping we'd already caught all of the #1s in 421892a19, but I
> caught a few of those in some of your other patches. One you'd done
> another way and some you'd done the rescope but just put it in the
> wrong patch.  The others had not been done yet. I just pushed
> f959bf9a5 to fix those ones.

This fixed pg_get_statisticsobj_worker() but not pg_get_indexdef_worker() nor
pg_get_partkeydef_worker().

(Also, I'd mentioned that my fixes for those deliberately re-used the
outer-scope vars, which isn't what you did, and it's why I didn't include them
with the patch for inner-scope).

> I really think #2s should be done last. I'm not as comfortable with
> the renaming and we might want to discuss tactics on that. We could
> either opt to rename the shadowed or shadowing variable, or both.  If
> we rename the shadowing variable, then pending patches or forward
> patches could use the wrong variable.  If we rename the shadowed
> variable then it's not impossible that backpatching could go wrong
> where the new code intends to reference the outer variable using the
> newly named variable, but when that's backpatched it uses the variable
> with the same name in the inner scope.  Renaming both would make the
> problem more obvious.  I'm not sure which is best.  The answer may
> depend on how many lines the variable is in scope for. If it's just
> for a few lines then the hunk context would conflict and the committer
> would likely notice the issue when resolving the conflict.

Yes, the hope is to limit the change to variables that are only used a couple
times within a few lines.  It's also possible that these will break patches in
development, but that's normal for any change at all.

> I'll study #7 a bit more. My eyes glazed over a bit from doing all
> that analysis, so I might be mistaken about that being a bug.

I reported this last week.
https://www.postgresql.org/message-id/20220819211824.GX26426@telsasoft.com

-- 
Justin

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