On Mon, Dec 9, 2019 at 11:48 AM Tom Lane <t...@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
