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

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.

0001. I'd also rather see these 4 renamed:

+++ b/src/bin/pg_dump/pg_dump.c
@@ -3144,7 +3144,6 @@ dumpDatabase(Archive *fout)
  PQExpBuffer loHorizonQry = createPQExpBuffer();
  int i_relfrozenxid,
  i_relfilenode,
- i_oid,
  i_relminmxid;

Adding an extra 'i' (for inner) on the front seems fine to me.

0002. I don't really like the "my" name. I also see you've added the
word "this" to many other variables that are shadowing. It feels kinda
like you're missing a "self" and a "me" in there somewhere! :)

@@ -7080,21 +7080,21 @@ getConstraints(Archive *fout, TableInfo
tblinfo[], int numTables)
  appendPQExpBufferChar(tbloids, '{');
  for (int i = 0; i < numTables; i++)
  {
- TableInfo  *tbinfo = &tblinfo[i];
+ TableInfo  *mytbinfo = &tblinfo[i];

How about just "tinfo"?

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

@@ -16799,21 +16799,21 @@ dumpSequence(Archive *fout, const TableInfo *tbinfo)
  */
  if (OidIsValid(tbinfo->owning_tab) && !tbinfo->is_identity_sequence)
  {
- TableInfo  *owning_tab = findTableByOid(tbinfo->owning_tab);
+ TableInfo  *this_owning_tab = findTableByOid(tbinfo->owning_tab);

0004. I would rather people used foreach_current_index(lc) > 0 to
determine when we're not doing the first iteration of a foreach loop.
I understand there are more complex cases with filtering that this
cannot be done, but these are highly simple and using
foreach_current_index() removes multiple lines of code and makes it
look nicer.

@@ -762,8 +762,8 @@ fetch_remote_table_info(char *nspname, char *relname,
  TupleTableSlot *slot;
  Oid attrsRow[] = {INT2VECTOROID};
  StringInfoData pub_names;
- bool first = true;

+ first = true;
  initStringInfo(&pub_names);
  foreach(lc, MySubscription->publications)

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

+++ b/src/backend/replication/logical/tablesync.c
@@ -759,7 +759,7 @@ fetch_remote_table_info(char *nspname, char *relname,
  if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 150000)
  {
  WalRcvExecResult *pubres;
- TupleTableSlot *slot;
+ TupleTableSlot *thisslot;

0006. A see the outer shadowed counterpart is used to add a new backup
type.  Since I'm not a fan of "this", how about the outer one gets
renamed to "newtype"?

+++ b/src/backend/backup/basebackup_target.c
@@ -73,9 +73,9 @@ BaseBackupAddTarget(char *name,
  /* Search the target type list for an existing entry with this name. */
  foreach(lc, BaseBackupTargetTypeList)
  {
- BaseBackupTargetType *ttype = lfirst(lc);
+ BaseBackupTargetType *this_ttype = lfirst(lc);

0007. Meh, more "this". How about just "col".

+++ b/src/backend/parser/parse_jsontable.c
@@ -341,13 +341,13 @@ transformJsonTableChildPlan(JsonTableContext
*cxt, JsonTablePlan *plan,
  /* transform all nested columns into cross/union join */
  foreach(lc, columns)
  {
- JsonTableColumn *jtc = castNode(JsonTableColumn, lfirst(lc));
+ JsonTableColumn *thisjtc = castNode(JsonTableColumn, lfirst(lc));

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.

0008. Sorry, I had to change this one too. I just have an aversion to
variables named "temp" or "tmp".

+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -3109,10 +3109,10 @@ JsonItemFromDatum(Datum val, Oid typid, int32
typmod, JsonbValue *res)

  if (JsonContainerIsScalar(&jb->root))
  {
- bool res PG_USED_FOR_ASSERTS_ONLY;
+ bool tmp PG_USED_FOR_ASSERTS_ONLY;

- res = JsonbExtractScalar(&jb->root, jbv);
- Assert(res);
+ tmp = JsonbExtractScalar(&jb->root, jbv);
+ Assert(tmp);

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?

David