Re: shadow variables - pg15 edition

Hi,

On 2022-10-06 13:00:41 +1300, David Rowley wrote:
> Here's a patch which (I think) fixes the ones I missed.

Yep, does the trick for me.

I attached a patch to add -Wshadow=compatible-local to our set of warnings.

> 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_); \

Not pretty. Given that HS_FINALIZE already has multiple-eval hazards, perhaps
we could just remove the local?

> --- 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;

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?

> --- 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)
> rv = NULL;
> else if (pg_strcasecmp(srv, "MODIFY") == 0)
> {
> - TriggerData *tdata = (TriggerData *) fcinfo->context;
> + TriggerData *trigdata = (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);
> + if (TRIGGER_FIRED_BY_INSERT(trigdata->tg_event) ||
> + TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event))
> + rv = PLy_modify_tuple(proc, plargs, trigdata, rv);
> else
> ereport(WARNING,
> (errmsg("PL/Python trigger function returned \"MODIFY\" in a DELETE trigger -- ignored")));

This doesn't strike me as a good fix either. Isn't the inner tdata exactly
the same as the outer tdata?

tdata = (TriggerData *) fcinfo->context;
...
TriggerData *trigdata = (TriggerData *) fcinfo->context;

> --- 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.

Anyway, this isn't important code, and your fix seem ok.

Greetings,

Andres Freund