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

On Thu, 6 Oct 2022 at 13:39, Andres Freund <andres@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