Date: Thu, 18 Aug 2022 18:21:41 -0500
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>
Subject: Re: shadow variables - pg15 edition
Message-ID: <20220818232141.GQ26426@telsasoft.com>
References: <20220817145434.GC26426@telsasoft.com>
 <CAApHDvqBBqF=wmV5azrO7h3VwpwQo+JFBQ+g=E6wVUhKcqR8gA@mail.gmail.com>
 <20220818051637.GK26426@telsasoft.com>
 <CAApHDvpP3z+Chsi00H5LMpQX0eybsz4v5BsS2Cqt3x5_Kc=8Kg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAApHDvpP3z+Chsi00H5LMpQX0eybsz4v5BsS2Cqt3x5_Kc=8Kg@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20220818232141.GQ26426%40telsasoft.com>
Precedence: bulk

On Thu, Aug 18, 2022 at 07:27:09PM +1200, David Rowley wrote:
> 0001. I'd also rather see these 4 renamed:
..
> 0002. I don't really like the "my" name. I also see you've added the
..
> How about just "tinfo"?
..
> 0005. How about just "tslot". I'm not a fan of "this".
..
> Since I'm not a fan of "this", how about the outer one gets renamed 
..
> 0007. Meh, more "this". How about just "col".
..
> 0008. Sorry, I had to change this one too.

I agree that ii_oid and newtype are better names (although it's a bit
unfortunate to rename the outer "ttype" var of wider scope).

> 0003. The following is used for the exact same purpose as its shadowed
> counterpart. I suggest just using the variable from the outer scope.

And that's what my original patch did, before people insisted that the patches
shouldn't change variable scope.  Now it's back to where I stared.

> There's a discussion about reverting this entire patch. Not sure if
> patching master and not backpatching to pg15 would be useful to the
> people who may be doing that revert.

I think if it were reverted, it'd be in both branches.

> I've attached a patch which does things more along the lines of how I
> would have done it.  I don't think we should be back patching this
> stuff.
> 
> Any objections to pushing this to master only?

I won't object, but some of your changes are what makes backpatching this less
reasonable (foreach_current_index and newtype).  I had made these v15 patches
first to simplify backpatching, since having the same code in v15 means that
there's no backpatch hazard for this new-in-v15 code.

I am opened to presenting the patches differently, but we need to come up with
a better process than one person writing patches and someone else rewriting it.
I also don't see the value of debating which order to write the patches in.
Grouping by variable name or doing other statistical analysis doesn't change
the fact that there are 50+ issues to address to allow -Wshadow to be usable.

Maybe these would be helpful ?
 - if I publish the patches on github;
 - if I send the patches with more context;
 - if you have an suggestion/objection/complaint with a patch, I can address it
   and/or re-arrange the patchset so this is later, and all the polished
   patches are presented first.

-- 
Justin