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