Re: shadow variables - pg15 edition

From: Justin Pryzby <pryzby(at)telsasoft(dot)com>
To: David Rowley <dgrowleyml(at)gmail(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org, Tomas Vondra <tomas(dot)vondra(at)postgresql(dot)org>, Peter Smith <smithpb2250(at)gmail(dot)com>
Subject: Re: shadow variables - pg15 edition
Date: 2022-08-18 23:21:41
Message-ID: 20220818232141.GQ26426@telsasoft.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Geoghegan 2022-08-18 23:50:37 Re: Remaining case where reltuples can become distorted across multiple VACUUM operations
Previous Message Peter Smith 2022-08-18 23:05:57 Re: Perform streaming logical transactions by background workers and parallel apply