Re: [Proposal] Level4 Warnings show many shadow vars

Hi,

On 2019-12-09 11:59:23 -0500, Robert Haas wrote:
> On Mon, Dec 9, 2019 at 11:48 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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.

+1. For me it leaves mildly bad taste seing code like that.

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

We do have a few files where we have names starting with underscores
ourselves, imo not a great idea for most of them.

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

FWIW, I've had bugs in code submitted to PG (both by myself and me
merging other people's work IIRC) that were related to such naming
conflicts.

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

It's also not always possible in C99, as we have plenty macros with
essentially dynamic types. And there's no typeof() in standard C,
unfortunately (C11's _Generic can help, but isn't great either).

Greetings,

Andres Freund