Re: [Proposal] Level4 Warnings show many shadow vars

Greetings,

Didn't see this previously (it's our typical approach to 'reply-all' to
people), though I don't think it changes my feelings about the latest
proposed patch.

* Ranier Vilela (ranier_gyn(at)hotmail(dot)com) wrote:
> De: Stephen Frost
> Enviadas: Terça-feira, 10 de Dezembro de 2019 17:52
>
> >There's multiple ways to get there though and I think what you're seeing
> >is that the "just change it to something else" answer isn't necessairly
> >going to be viewed as an improvement (or, at least, not enough of an
> >improvement to accept the cost of the change).
> Well, I was trying to apply another non-implied rule, "break nothing".

I agree with not breaking things but that doesn't mean the only
reasonable approach is to do the absolute minimum- you might not be
breaking something today, but it's going to confuse people later on down
the road and may lead to bugs being introduced due to that confusion, or
at the very least will add to people's time to figure out what's really
going on.

> >Why not change the variables? Changes that also improve the code itself
> >along with eliminating the shadowing of the global variable are going to
> >be a lot easier to be accepted.
> Contrary to what I was initially thinking, it seems to me that changing the names of global variables is more acceptable to the people of the project.

I wasn't suggesting to change the names of the global variables in this
specific case, though I could see that being a better approach in some
instances- but it really depends. Each case needs to be reviewed and
considered and the best approach taken.

> >Sure, but have you looked at how it's used? Instead of just renaming
> >the numTables variables in the functions that accept it- could those
> >variables just be removed instead of changing their name to make it look
> >like they're something different when they aren't actually different?
> No. I didn't look.

I think we need to be looking at the changes and considering them, and
the person proposing the changes should be doing that and not just
expecting everyone else to do so.

> >I've only spent a bit of time looking at it, but it sure looks like the
> >variables could just be removed, and doing so doesn't break the
> >regression tests, which supports the idea that maybe there's a better
> >way to deal with those particular variables rather than renaming them.
> >Another approach to consider might be to move some global variables into
> >structures that are then global with better names to indicate that's
> >what they are.
> It does not seem reasonable to me what you are asking.
> Because as I was told here and I agree in part. I do not have the necessary knowledge of structures and logic to propose big changes.

I'd suggest that we work through that then and get you up to speed on
the structures and logic- the pg_dump code is pretty ugly but the
specific usage of numTables isn't too bad. Each of these should be
looked at independently and thought about "what's the right way to fix
this?" The right way isn't necessairly to just rename the variables, as
I was saying, and doing so may lead to more confusion, not less.

> For the work I set out to, find bugs and make minor performance improvements, I believe, can contribute safely and without ruining anything.

Having shadowed globals, while kinda ugly, doesn't necessairly mean it's
a bug. I'm not sure what "minor performance improvements" are being
claimed here but there's a whole lot of work involved in demonstrating
that a change is a performance improvement.

> By just changing the names of variables to something consistent and readable, the goal will be done without break anything.

but.. the changes you're proposing are making them inconsistent and
confusing when there isn't actually a difference between the global and
the local, it's just the somewhere along the way someone thought they
needed to pass in numTables when they really didn't, and we should go
fix *that*, not rename the variable to something else to make someone
later on go "wait, why did we need to pass in this variable? how is
this different from the global?"

> Who is best to make these changes, are the authors and reviewers. Once we no longer have the problem of shadow variables, we can turn on the alert without breaking automatic compilation, as Tom Lane is concerned.

Perhaps I'm a bit confused, but it seems that you're the author of these
specific changes, and I'm trying to provide feedback as to how you can
improve what you're proposing in a way that will improve the code base
overall and reduce the confusion while also eliminating the shadow
variables. If the author of a patch isn't open to this kind of review
and willing to adjust the patch to improve it because they aren't sure
that their changes will be correct then they could at least post them
back here and ask, or better, go look at the code and get a better
understanding of what's going on to build confidence in the change.

The goal here also shouldn't be "we just want to turn on this alert, so
we're going to make changes to the source without thinking just to
appease the compiler".

> That's why I'd like to fix all collisions of variables, even the dumbest.

I agree with fixing collisions, but not in a rote way like this.

> >In short, a hack-and-slash patch that doesn't really spend much time
> >considering the changes beyond "let's just change these to be different
> >to avoid shadowing globals" isn't really a good way to go about
> >addressing these cases and has a good chance of making things more
> >confusing, not less.
> This is totally contrary to what I think about it.

-1 from me then on this whole thread of changes.

Thanks,

Stephen