On Thu, 18 Aug 2022 at 02:54, Justin Pryzby <pry...@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
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index ba3532a51e..6d285255dd 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -56,22 +56,22 @@
#define EXPECT_EQ_U32(result_expr, expected_expr) \
do { \
- uint32 result = (result_expr); \
- uint32 expected = (expected_expr); \
- if (result != expected) \
+ uint32 actual_result = (result_expr); \
+ uint32 expected_result = (expected_expr); \
+ if (actual_result != expected_result) \
elog(ERROR, \
"%s yielded %u, expected %s in file \"%s\"
line %u", \
- #result_expr, result, #expected_expr,
__FILE__, __LINE__); \
+ #result_expr, actual_result, #expected_expr,
__FILE__, __LINE__); \
} while (0)
#define EXPECT_EQ_U64(result_expr, expected_expr) \
do { \
- uint64 result = (result_expr); \
- uint64 expected = (expected_expr); \
- if (result != expected) \
+ uint64 actual_result = (result_expr); \
+ uint64 expected_result = (expected_expr); \
+ if (actual_result != expected_result) \
elog(ERROR, \
"%s yielded " UINT64_FORMAT ", expected %s in
file \"%s\" line %u", \
- #result_expr, result, #expected_expr,
__FILE__, __LINE__); \
+ #result_expr, actual_result, #expected_expr,
__FILE__, __LINE__); \
} while (0)
#define LDELIM '('
