Re: [Proposal] Level4 Warnings show many shadow vars

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Dec 9, 2019 at 10:23 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Well, again, this is a case-by-case question.  I tend to agree that
>> changing that usage in subscriptioncmds.c might be a good idea.
>> That doesn't mean I need to be on board with wholesale removal
>> of shadowing warnings.

> I agree that those things are different, but I'm not sure I understand
> the nuances of your view. I think my view is that if something in our
> code is shadowing something else in our code, that's probably
> something we ought to look at fixing. If you disagree, I'd be curious
> to know why; I suspect that, as in this case, such cases are just
> creating a risk of confusion without any real benefit.

I think it depends a lot on the particular identifiers in use.  You
mentioned examples like "i" and "lc", and I'd add other obviously
nonce variable names like "oldcxt".  I'm not particularly concerned
about shadowing arising from somebody writing a five-line loop using
a local "i" inside a much larger loop also using "i" --- yeah, in
theory there could be an issue, but in practice there isn't.  Being
picky about that just adds difficulty when writing/reviewing a patch
that adds such a five-line loop.

Your point about risking macro breakage from shadowing of global
variable names is a good one, but again I don't think it holds up
as an argument that we have to get rid of all shadowing.

> To me, the grey
> area is in conflicts between stuff in our code and stuff in system
> header files. I'm not sure I'd want to try to have precisely 0
> conflicts with every crazy decision by every OS / libc maintainer out
> there, and I suspect on that point at least we are in agreement.

I believe the relevant C standards (and practice) are that random
names exposed by system headers ought to start with some underscores.
If there's a conflict there, it's a bug in the header and cause
for a bug report to the OS vendor, not us.

Now, if a conflict of that sort exists and is causing a live bug in PG
on a popular OS, then I'd likely be on board with adjusting our code
to dodge the problem.  But not with doing so just to silence a
compiler warning.

A final point here is that in practice, we've had way more problems
with conflicts against system headers' definitions of functions,
macros, and typedefs than global variables, which is unsurprising
considering how few of the latter are actually exported by typical
C library APIs.  So I'm not sure that there is any big problem to
be solved there in the first place.

The only thing I think is really a substantial bug risk here is your
point about our own macros referencing our own global variables.
We might be better off fixing that in a localized way by establishing
a policy that any such macros should be converted to static inlines.

            regards, tom lane