Re: [Proposal] Level4 Warnings show many shadow vars

Greetings,

* Ranier Vilela (ranier_gyn(at)hotmail(dot)com) wrote:
> >For someone that expounds consistency - this patch is the furthest thing from it.
> >In some places names are randomly changed to have an underscore >(authmethodlocal to authmethod_local with the obvious inconsistency as well) - >in some places names are changed to remove underscores (stop_t to stopt). >Some places random letters are added (checkPoint to xcheckPoint) some places >perfectly good names are truncated (conf_file to file).
> The first purpose of the patch was to remove collisions from shadow global variable names.

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).

> The second was not to change the semantics of variable names, hence the use of x or putting or remove underscore.

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.

> >Random places remove perfectly good prefixes and replace with single letters >(numTables to nTables)
> numTables already a global variable name.

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?

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.

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.

Thanks,

Stephen