Re: shadow variables - pg15 edition

On Wed, 24 Aug 2022 at 14:39, Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
> Attached are half of the remainder of what I've written, ready for review.

Thanks for the patches.

I started to do some analysis of the remaining warnings and put them
in the attached spreadsheet. I put each of the remaining warnings into
a category of how I think they should be fixed.

These categories are:

1. "Rescope" (adjust scope of outer variable to move it into a deeper scope)
2. "Rename" (a variable needs to be renamed)
3. "RenameOrScope" (a variable needs renamed or we need to something
more extreme to rescope)
4. "Repurpose" (variables have the same purpose and may as well use
the same variable)
5. "Refactor" (fix the code to make it better)
6. "Remove" (variable is not needed)

There's also:
7. "Bug?" (might be a bug)
8. "?" (I don't know)

I was hoping we'd already caught all of the #1s in 421892a19, but I
caught a few of those in some of your other patches. One you'd done
another way and some you'd done the rescope but just put it in the
wrong patch. The others had not been done yet. I just pushed
f959bf9a5 to fix those ones.

I really think #2s should be done last. I'm not as comfortable with
the renaming and we might want to discuss tactics on that. We could
either opt to rename the shadowed or shadowing variable, or both. If
we rename the shadowing variable, then pending patches or forward
patches could use the wrong variable. If we rename the shadowed
variable then it's not impossible that backpatching could go wrong
where the new code intends to reference the outer variable using the
newly named variable, but when that's backpatched it uses the variable
with the same name in the inner scope. Renaming both would make the
problem more obvious. I'm not sure which is best. The answer may
depend on how many lines the variable is in scope for. If it's just
for a few lines then the hunk context would conflict and the committer
would likely notice the issue when resolving the conflict.

For #3, I just couldn't decide the best fix. Many of these could be
moved into an inner scope, but it would require indenting a large
amount of code, e.g. in a switch() statement's "case:" to allow
variables to be declared within the case.

I think probably #4 should be next to do (maybe after #5)

I have some ideas on how to fix the two #5s, so I'm going to go and do that now.

There's only 1 #6. I'm not so sure on that yet. The variable being
assigned to the variable is the current time and I'm not sure if we
can reuse the existing variable or not as time may have moved on
sufficiently.

I'll study #7 a bit more. My eyes glazed over a bit from doing all
that analysis, so I might be mistaken about that being a bug.

For #8s. These are the PG_TRY() ones. I see you had a go at fixing
that by moving the nested PG_TRY()s to a helper function. I don't
think that's a good fix. If we were to ever consider making
-Wshadow=compatible-local in a standard build, then we'd basically be
saying that nested PG_TRYs are not allowed. I don't think that'll fly.
I'd rather find a better way to fix those. I see we can't make use of
##__LINE__ in the variable name since PG_TRY()'s friends use the
variables too and they'd be on a different line. We maybe could have
an "ident" parameter in the macro that we ##ident onto the variables
names, but that would break existing code.

> The first patch removes 2ndary, "inner" declarations, where that seems
> reasonably safe and consistent with existing practice (and probably what the
> original authors intended or would have written).

Would you be able to write a patch for #4. I'll do #5 now. You could
do a draft patch for #2 as well, but I think it should be committed
last, if we decide it's a good move to make. It may be worth having
the discussion about if we actually want to run
-Wshadow=compatible-local as a standard build flag before we rename
anything.

David