On Thu, 6 Oct 2022 at 13:39, Andres Freund <and...@anarazel.de> wrote:
> I attached a patch to add -Wshadow=compatible-local to our set of warnings.
Thanks for writing that and for looking at the patch.
FWIW, I'm +1 for having this part of our default compilation flags. I
don't want to have to revisit this on a yearly basis. I imagine Justin
doesn't want to do that either. I feel that since this work has
already uncovered 2 existing bugs that it's worth having this as a
default compilation flag. Additionally, in the cases like in the
PLy_exec_trigger() trigger case below, I feel this has resulted in
slightly more simple code that's easier to follow. I feel having to be
slightly more inventive with variable names in a small number of cases
is worth the trouble. I feel the cases where this could get annoying
are probably limited to variables declared in macros. Maybe that's
just a reason to consider static inline functions instead. That
wouldn't work for macros such as PG_TRY(), but I think macros in that
category are rare. I think switching it on does not mean we can never
switch it off again should we ever find something we're unable to work
around. That just seems a little unlikely given that with the prior
commits plus the attached patch, we've managed to fix ~30 years worth
of opportunity to introduce shadowed local variables.
> > diff --git a/contrib/hstore/hstore.h b/contrib/hstore/hstore.h
> > #define HS_FINALIZE(hsp_,count_,buf_,ptr_)
> > \
> > do {
> > \
> > - int buflen = (ptr_) - (buf_);
> > \
> > + int _buflen = (ptr_) - (buf_);
> > \
>
> Not pretty. Given that HS_FINALIZE already has multiple-eval hazards, perhaps
> we could just remove the local?
You're right. It's not that pretty, but I don't feel like making the
hazards any worse is a good idea. This is old code. I'd rather change
it as little as possible to minimise the risk of introducing any bugs.
I'm open to other names for the variable, but I just don't want to
widen the scope for multiple evaluation hazards.
> > --- a/src/interfaces/libpq/fe-secure-gssapi.c
> > +++ b/src/interfaces/libpq/fe-secure-gssapi.c
> > @@ -135,11 +135,11 @@ pg_GSS_write(PGconn *conn, const void *ptr, size_t
> > len)
> > - ssize_t ret;
> > + ssize_t retval;
>
> That looks like it could easily lead to confusion further down the
> line. Wouldn't the better fix here be to remove the inner variable?
hmm. You're maybe able to see something I can't there, but to me, it
looks like reusing the outer variable could change the behaviour of
the function. Note at the end of the function we set "ret" just
before the goto label. It looks like it might be possible for the
goto to jump to the point after "ret = bytes_sent;", in which case we
should return -1, the default value for the outer "ret". If I go and
reuse the outer "ret" for something else then it'll return whatever
value it's left set to. I could study the code more and perhaps work
out that that cannot happen, but if it can't then it's really not
obvious to me and if it's not obvious then I just don't feel the need
to take any undue risks by reusing the outer variable. I'm open to
better names, but I'd just rather not reuse the outer scoped variable.
> > --- a/src/pl/plpython/plpy_exec.c
> > +++ b/src/pl/plpython/plpy_exec.c
> > @@ -375,11 +375,11 @@ PLy_exec_trigger(FunctionCallInfo fcinfo,
> > PLyProcedure *proc)
> > - TriggerData *tdata = (TriggerData *)
> > fcinfo->context;
> > + TriggerData *trigdata = (TriggerData *)
> > fcinfo->context;
> This doesn't strike me as a good fix either. Isn't the inner tdata exactly
> the same as the outer data?
Yeah, you're right. I've adjusted the patch to use the outer scoped
variable and get rid of the inner scoped one.
> > --- a/src/test/modules/test_integerset/test_integerset.c
> > +++ b/src/test/modules/test_integerset/test_integerset.c
> > @@ -585,26 +585,26 @@ test_huge_distances(void)
>
> This is one of the cases where our insistence on -Wdeclaration-after-statement
> really makes this unnecessary ugly... Declaring x at the start of the function
> just makes this harder to read.
Yeah, it's not pretty. Maybe one day we'll relax that rule. Until
then, I think it's not worth expending too much thought on a test
module.
David
diff --git a/contrib/bloom/blinsert.c b/contrib/bloom/blinsert.c
index e64291e049..dd26d6ac29 100644
--- a/contrib/bloom/blinsert.c
+++ b/contrib/bloom/blinsert.c
@@ -232,8 +232,6 @@ blinsert(Relation index, Datum *values, bool *isnull,
if (metaData->nEnd > metaData->nStart)
{
- Page page;
-
blkno = metaData->notFullPage[metaData->nStart];
Assert(blkno != InvalidBlockNumber);
diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c
index de0b9a109c..67821cd25b 100644
--- a/contrib/file_fdw/file_fdw.c
+++ b/contrib/file_fdw/file_fdw.c
@@ -450,15 +450,15 @@ get_file_fdw_attribute_options(Oid relid)
for (attnum = 1; attnum <= natts; attnum++)
{
Form_pg_attribute attr = TupleDescAttr(tupleDesc, attnum - 1);
- List *options;
+ List *column_options;
ListCell *lc;
/* Skip dropped attributes. */
if (attr->attisdropped)
continue;
- options = GetForeignColumnOptions(relid, attnum);
- foreach(lc, options)
+ column_options = GetForeignColumnOptions(relid, attnum);
+ foreach(lc, column_options)
{
DefElem *def = (DefElem *) lfirst(lc);
@@ -480,7 +480,7 @@ get_file_fdw_attribute_options(Oid relid)
fncolumns = lappend(fncolumns,
makeString(attname));
}
}
- /* maybe in future handle other options here */
+ /* maybe in future handle other column options here */
}
}
diff --git a/contrib/hstore/hstore.h b/contrib/hstore/hstore.h
index 4713e6ea7a..897af244a4 100644
--- a/contrib/hstore/hstore.h
+++ b/contrib/hstore/hstore.h
@@ -128,15 +128,15 @@ typedef struct
/* finalize a newly-constructed hstore */
#define HS_FINALIZE(hsp_,count_,buf_,ptr_)
\
do {
\
- int buflen = (ptr_) - (buf_);
\
+ int _buflen = (ptr_) - (buf_);
\
if ((count_))
\
ARRPTR(hsp_)[0].entry |= HENTRY_ISFIRST;
\
if ((count_) != HS_COUNT((hsp_)))
\
{
\
HS_SETCOUNT((hsp_),(count_));
\
- memmove(STRPTR(hsp_), (buf_), buflen);
\
+ memmove(STRPTR(hsp_), (buf_), _buflen);
\
}
\
- SET_VARSIZE((hsp_), CALCDATASIZE((count_), buflen));
\
+ SET_VARSIZE((hsp_), CALCDATASIZE((count_), _buflen));
\
} while (0)
/* ensure the varlena size of an existing hstore is correct */
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 09f37fb77a..9524765650 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -943,8 +943,6 @@ foreign_expr_walker(Node *node,
*/
if (agg->aggorder)
{
- ListCell *lc;
-
foreach(lc, agg->aggorder)
{
SortGroupClause *srt =
(SortGroupClause *) lfirst(lc);
diff --git a/contrib/postgres_fdw/postgres_fdw.c
b/contrib/postgres_fdw/postgres_fdw.c
index dd858aba03..8d013f5b1a 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -1341,8 +1341,6 @@ postgresGetForeignPlan(PlannerInfo *root,
*/
if (outer_plan)
{
- ListCell *lc;
-
/*
* Right now, we only consider grouping and aggregation
beyond
* joins. Queries involving aggregates or grouping do
not require
@@ -6272,10 +6270,10 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo
*grouped_rel,
*/
foreach(l, aggvars)
{
- Expr *expr = (Expr *) lfirst(l);
+ Expr *aggref = (Expr *) lfirst(l);
- if (IsA(expr, Aggref))
- tlist =
add_to_flat_tlist(tlist, list_make1(expr));
+ if (IsA(aggref, Aggref))
+ tlist =
add_to_flat_tlist(tlist, list_make1(aggref));
}
}
}
@@ -6289,8 +6287,6 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo
*grouped_rel,
*/
if (havingQual)
{
- ListCell *lc;
-
foreach(lc, (List *) havingQual)
{
Expr *expr = (Expr *) lfirst(lc);
@@ -6324,7 +6320,6 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo
*grouped_rel,
if (fpinfo->local_conds)
{
List *aggvars = NIL;
- ListCell *lc;
foreach(lc, fpinfo->local_conds)
{
diff --git a/src/interfaces/libpq/fe-secure-gssapi.c
b/src/interfaces/libpq/fe-secure-gssapi.c
index 6ea52ed866..dee0982eba 100644
--- a/src/interfaces/libpq/fe-secure-gssapi.c
+++ b/src/interfaces/libpq/fe-secure-gssapi.c
@@ -135,11 +135,11 @@ pg_GSS_write(PGconn *conn, const void *ptr, size_t len)
*/
if (PqGSSSendLength)
{
- ssize_t ret;
+ ssize_t retval;
ssize_t amount = PqGSSSendLength -
PqGSSSendNext;
- ret = pqsecure_raw_write(conn, PqGSSSendBuffer +
PqGSSSendNext, amount);
- if (ret <= 0)
+ retval = pqsecure_raw_write(conn, PqGSSSendBuffer +
PqGSSSendNext, amount);
+ if (retval <= 0)
{
/*
* Report any previously-sent data; if there
was none, reflect
@@ -149,16 +149,16 @@ pg_GSS_write(PGconn *conn, const void *ptr, size_t len)
*/
if (bytes_sent)
return bytes_sent;
- return ret;
+ return retval;
}
/*
* Check if this was a partial write, and if so, move
forward that
* far in our buffer and try again.
*/
- if (ret != amount)
+ if (retval != amount)
{
- PqGSSSendNext += ret;
+ PqGSSSendNext += retval;
continue;
}
diff --git a/src/pl/plpython/plpy_cursorobject.c
b/src/pl/plpython/plpy_cursorobject.c
index 6b6e743345..57e8f8ec21 100644
--- a/src/pl/plpython/plpy_cursorobject.c
+++ b/src/pl/plpython/plpy_cursorobject.c
@@ -215,18 +215,18 @@ PLy_cursor_plan(PyObject *ob, PyObject *args)
PyObject *elem;
elem = PySequence_GetItem(args, j);
- PG_TRY();
+ PG_TRY(2);
{
bool isnull;
plan->values[j] = PLy_output_convert(arg, elem,
&isnull);
nulls[j] = isnull ? 'n' : ' ';
}
- PG_FINALLY();
+ PG_FINALLY(2);
{
Py_DECREF(elem);
}
- PG_END_TRY();
+ PG_END_TRY(2);
}
portal = SPI_cursor_open(NULL, plan->plan, plan->values, nulls,
diff --git a/src/pl/plpython/plpy_exec.c b/src/pl/plpython/plpy_exec.c
index 150b3a5977..923703535a 100644
--- a/src/pl/plpython/plpy_exec.c
+++ b/src/pl/plpython/plpy_exec.c
@@ -375,8 +375,6 @@ PLy_exec_trigger(FunctionCallInfo fcinfo, PLyProcedure
*proc)
rv = NULL;
else if (pg_strcasecmp(srv, "MODIFY") == 0)
{
- TriggerData *tdata = (TriggerData *)
fcinfo->context;
-
if (TRIGGER_FIRED_BY_INSERT(tdata->tg_event) ||
TRIGGER_FIRED_BY_UPDATE(tdata->tg_event))
rv = PLy_modify_tuple(proc, plargs,
tdata, rv);
diff --git a/src/pl/plpython/plpy_spi.c b/src/pl/plpython/plpy_spi.c
index 9a71a42c15..6b9f8d5b43 100644
--- a/src/pl/plpython/plpy_spi.c
+++ b/src/pl/plpython/plpy_spi.c
@@ -236,18 +236,18 @@ PLy_spi_execute_plan(PyObject *ob, PyObject *list, long
limit)
PyObject *elem;
elem = PySequence_GetItem(list, j);
- PG_TRY();
+ PG_TRY(2);
{
bool isnull;
plan->values[j] = PLy_output_convert(arg, elem,
&isnull);
nulls[j] = isnull ? 'n' : ' ';
}
- PG_FINALLY();
+ PG_FINALLY(2);
{
Py_DECREF(elem);
}
- PG_END_TRY();
+ PG_END_TRY(2);
}
rv = SPI_execute_plan(plan->plan, plan->values, nulls,
diff --git a/src/test/modules/test_integerset/test_integerset.c
b/src/test/modules/test_integerset/test_integerset.c
index 578d2e8aec..813ca4ba6b 100644
--- a/src/test/modules/test_integerset/test_integerset.c
+++ b/src/test/modules/test_integerset/test_integerset.c
@@ -585,26 +585,26 @@ test_huge_distances(void)
*/
for (int i = 0; i < num_values; i++)
{
- uint64 x = values[i];
+ uint64 y = values[i];
bool expected;
bool result;
- if (x > 0)
+ if (y > 0)
{
- expected = (values[i - 1] == x - 1);
- result = intset_is_member(intset, x - 1);
+ expected = (values[i - 1] == y - 1);
+ result = intset_is_member(intset, y - 1);
if (result != expected)
- elog(ERROR, "intset_is_member failed for "
UINT64_FORMAT, x - 1);
+ elog(ERROR, "intset_is_member failed for "
UINT64_FORMAT, y - 1);
}
- result = intset_is_member(intset, x);
+ result = intset_is_member(intset, y);
if (result != true)
- elog(ERROR, "intset_is_member failed for "
UINT64_FORMAT, x);
+ elog(ERROR, "intset_is_member failed for "
UINT64_FORMAT, y);
- expected = (i != num_values - 1) ? (values[i + 1] == x + 1) :
false;
- result = intset_is_member(intset, x + 1);
+ expected = (i != num_values - 1) ? (values[i + 1] == y + 1) :
false;
+ result = intset_is_member(intset, y + 1);
if (result != expected)
- elog(ERROR, "intset_is_member failed for "
UINT64_FORMAT, x + 1);
+ elog(ERROR, "intset_is_member failed for "
UINT64_FORMAT, y + 1);
}
/*
