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

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