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

On Fri, Aug 19, 2022 at 9:21 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> 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.
>

Starting off with patches might come to grief, and it won't be much
fun rearranging patches over and over.

Because there are so many changes, I think it would be better to
attack this task methodically:

STEP 1 - Capture every shadow warning and categorise exactly what kind
is it. e.g maybe do this as some XLS which can be shared. The last
time I looked there were hundreds of instances, but I expect there
will be less than a couple of dozen different *categories* of them.

e.g. shadow of a global var
e.g. shadow of a function param
e.g. shadow of a function var in a code block for the exact same usage
e.g. shadow of a function var in a code block for some 'tmp' var
e.g. shadow of a function var in a code block due to a mistake
e.g. shadow of a function var by some loop index
e.g. shadow of a function var for some loop 'first' handling
e.g. bug
etc...

STEP 2 - Define your rules for how intend to address each of these
kinds of shadows  (e.g. just simple rename of the var, use
'foreach_current_index', ...). Hopefully, it will be easy to reach an
agreement now since all instances of the same kind will look pretty
much the same.

STEP 3 - Fix all of the same kinds of shadows per single patch (using
the already agreed fix approach from step 2).

REPEAT STEPS 2,3 until done.

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