Re: shadow variables - pg15 edition

On Thu, 18 Aug 2022 at 02:54, Justin Pryzby <pryzby@telsasoft.com> wrote:
> The first half of the patches fix shadow variables newly-introduced in v15
> (including one of my own patches), the rest are fixing the lowest hanging fruit
> of the "short list" from COPT=-Wshadow=compatible-local

I wonder if it's better to fix the "big hitters" first.  The idea
there would be to try to reduce the number of these warnings as
quickly and easily as possible.  If we can get the numbers down fairly
significantly without too much effort, then that should provide us
with a bit more motivation to get rid of the remaining ones.

Here are the warnings grouped by the name of the variable:

$ make -s 2>&1 | grep "warning: declaration of" | grep -oP
"‘([_a-zA-Z]{1}[_a-zA-Z0-9]*)’" | sort | uniq -c
      2 ‘aclresult’
      3 ‘attnum’
      1 ‘cell’
      1 ‘cell__state’
      2 ‘cmp’
      2 ‘command’
      1 ‘constraintOid’
      1 ‘copyTuple’
      1 ‘data’
      1 ‘db’
      1 ‘_do_rethrow’
      1 ‘dpns’
      1 ‘econtext’
      1 ‘entry’
     36 ‘expected’
      1 ‘first’
      1 ‘found_whole_row’
      1 ‘host’
     20 ‘i’
      1 ‘iclause’
      1 ‘idxs’
      1 ‘i_oid’
      4 ‘isnull’
      1 ‘it’
      2 ‘item’
      1 ‘itemno’
      1 ‘j’
      1 ‘jtc’
      1 ‘k’
      1 ‘keyno’
      7 ‘l’
     13 ‘lc’
      4 ‘lc__state’
      1 ‘len’
      1 ‘_local_sigjmp_buf’
      1 ‘name’
      2 ‘now’
      1 ‘owning_tab’
      1 ‘page’
      1 ‘partitionId’
      2 ‘path’
      3 ‘proc’
      1 ‘proclock’
      1 ‘querytree_list’
      1 ‘range’
      1 ‘rel’
      1 ‘relation’
      1 ‘relid’
      1 ‘rightop’
      2 ‘rinfo’
      1 ‘_save_context_stack’
      1 ‘save_errno’
      1 ‘_save_exception_stack’
      1 ‘slot’
      1 ‘sqlca’
      9 ‘startelem’
      1 ‘stmt_list’
      2 ‘str’
      1 ‘subpath’
      1 ‘tbinfo’
      1 ‘ti’
      1 ‘transno’
      1 ‘ttype’
      1 ‘tuple’
      5 ‘val’
      1 ‘value2’
      1 ‘wco’
      1 ‘xid’
      1 ‘xlogfname’

The top 5 by count here account for about half of the warnings, so
maybe is best to start with those? Likely the ones ending in __state
will fix themselves when you fix the variable with the same name
without that suffix.

The attached patch targets fixing the "expected" variable.

$ ./configure --prefix=/home/drowley/pg
CFLAGS="-Wshadow=compatible-local" > /dev/null
$ make clean -s
$ make -j -s 2>&1 | grep "warning: declaration of" | wc -l
153
$ make clean -s
$ patch -p1 < reduce_local_variable_shadow_warnings_in_regress.c.patch
$ make -j -s 2>&1 | grep "warning: declaration of" | wc -l
117

So 36 fewer warnings with the attached.

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?

David