Re: [Proposal] Level4 Warnings show many shadow vars

On Mon, Dec 9, 2019 at 11:48 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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.

I think I would take the contrary view here. I think reusing the same
variable names in a single function is confusing, and if I noticed it
while reviewing, I would ask for it to be changed. It's not a
five-alarm fire, but it's not good, either.

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

Sure. I mean we'd have to look at individual cases, but in general I agree.

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

Sounds reasonable.

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

That would be a lot of work, but it would probably have some side
benefits, like making things more type-safe.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company