RE: [Proposal] Level4 Warnings show many shadow vars

De: Stephen Frost
Enviadas: Quarta-feira, 11 de Dezembro de 2019 18:57
>I really don't have any doubts that it's going to lead to confusion,
>particularly in a case like the numTables vs. nTables thing you're
>proposing in the one case that I spent some time looking at, and that
>confusion certainly could lead to bugs. Sure, having shadow variables
>also could- but you haven't identified an actual bug there today, so why
>not just fix it in a way that eliminates the confusion here?
I'm starting to think you're absolutely right.

>Here's an example of my concern- we change the name of the numTables
>variable in these pg_dump functions to nTables as you propose... And
>then later on someone starts hacking on these functions and they know
>about the global and they start using it, so now we've got two
>variables, both able to be used in the same scope, but one of them is a
>global and the other is a local. As long as both are always the same,
>sure everything works- but what happens if, for whatever reason, someone
>uses the function in a new way and passes in a different value as an
>argument, one that doesn't match what the global has? Well, some of the
>code will use the argument, and some of the code won't. At least today,
>there's no chance that the global variable will be used inside that
>function- it's *always* going to use the argument passed in.
Understood.

>I don't think that's even that far-fetched of a possibility considering
>most of the code is using the global variable directly and these
>functions are really the odd ones where numTables is being passed in as
>an argument, so ending up with a mix in the function looks rather likely
>to happen, and a bug resulting from that inconsistency entirely
>possible.
Yes.

>It's also possbile that the changes you're proposing might themselves
>induce bugs- by keeping the variable and just renaming it, you had
>better be ABSOLUTELY sure you rename every case because, if you don't,
>everything will still work just *fine*, except where you missed a case,
>the code will reference the global and the compiler won't complain and
>it might very well look like everything is working.
I can tell you that I tried to take every precaution in that direction.
But it is really not exempt from this risk.

>Either way, in my view, you had better review the code, have an
>understanding of how it works, and make sure that the change you're
>making is correct and makes sense, and that you've tested it well.
This view is very correct.

>Then I'd suggest that you spend some time looking at each case and
>working through what the best approach is and proposing patches that use
>the best approach in each case. If you don't wish to spend time on
>that, that's fine, but I don't agree with this approach of just pushing
>through and making things changes just to satisfy a particular compiler
>warning. I don't anticipate further discussion on this changing my mind
>on this point.

>I've already pointed out why I don't think this is the right approach to
>be addressing these issues, and it seems that you don't disagree with me
>about the recommended changes I've suggested, you've just said that you
>only want to think about or care about renaming of variables and I am
>specifically saying that's not an acceptable approach to addressing
>these issues.

>Why is that an issue?
It's not anymore.

>Feel free to monitor the situation and complain about new patches which
>are proposed that add to them. I don't have any particular problem with
>that. Nor do I object to generally pushing forward with the goal of
>eliminating the existing ones.
I would better it safer to do this automatically in the course of development.
Alone, the risk of failure is high.

>Let me lay this out in a different way- we could do the exact same thing
>you're doing here by just mindlessly changing, right before we commit,
>any variables that shadow global variables, we'd eliminate the compiler
>error, but it doesn't directly make anything *better* by itself, and
>ultimately isn't really all that different from the current situation
>where the compiler is essentially doing this for us by manging the
>variables as shadowing the globals thanks to C scoping rules, except
>that we add in the possibility of mixing usage of the local and the
>global throughout the functions therefore adding to the confusion.
This is the light.

>That doesn't actually make any of them bugs though.
Truth.

>I'm all for working to eliminate these shadow variables, but, again, not
>through rote renaming of the locals without putting in any real thought
>about what the code is doing and working out what the right approach to
>such a change to eliminate the shadow variables should be.
>Ok, so this specific "global unshadow patch" is all about bugs, but
>without actually showing that there's actual bugs here, just that there
>are shadowed variables... In which case, the real question is "is this
>change an improvement to the code" and I'm arguing that just the act of
>changing the variable names to avoid shadowing isn't necessairly a code
>improvement- that has to be evaluated on a case-by-case basis. If
>you're not going to do that evaluation then you're just throwing changes
>at the community with the expectation that someone else will do the
>analysis and decide if the changes are worthwhile or not and that
>strikes me as not really being very helpful. I'd really rather work
>towards patches that are clear improvements which have been well
>considered by the proposer of the patch.
Sorry for not being able to answer point by point, your considerations.
but I think I finally got to understand them correctly.
By renaming, we would be hiding the dirt under the carpet.
And that is absolutely what I want.
From your point of view and from John, the fact that the compiler warns us of the dangers of collisions is much better than simply turning them off by renaming them.
Seeing that, I have to accept.

1.So I would like to ask you if at least what has consensus could be used.
Or is it better to leave everything as it is?

2.About local shadow variables, would you find it safe to do redundant declaration removals of the type: int i? Is it worth it to work on that?

Best regards,
Ranier Vilela