Hi hackers,
Recently I was involved with some patches [1][2] to fix code which was
mistakenly using a global "wrconn" variable instead of a local one.
That bug led me to wonder if similar problems might be going
undetected elsewhere in the code. There is a gcc compiler option [3]
-Wshadow which informs about the similar scenario where one variable
is "shadowing" another (e.g. redeclaring a variable with the same name
as one at an outer scope).
PSA a log file from a PG14 build (code from last week) run using the
-Wshadow flag. In this logfile I have filtered out everything except
the shadow warnings.
My plan initially was to just fix the few warnings found and present
the patches here, but it turned out there are far more cases than I
was anticipating.
There seem to be basically 3 categories of shadowing exposed in this logfile:
1. where a var declaration is shadowing a previously declared local
var (205 cases found)
2. where a var declaration is shadowing a function parameter (14 cases found)
3. where a var declaration is shadowing a global variable (110 cases found)
~~~
Of the dozen or so cases that I have looked at, so far I have been
unable to find anything that would result in any *real* errors.
But that is not to say they are harmless either - at the very least
IMO they affect code readability in ways that span the full spectrum
from "meh" to downright "dodgy-looking".
Some examples are possibly deliberate (albeit lazy / unimaginative?)
local re-declarations of variables like "i" and "buf" etc.
But many other examples (particularly the global shadows) seemed
clearly unintentional mistakes to me - like the code evolved and
continued working OK without warnings, so any introduced shadowing
just went unnoticed.
And who knows... maybe there are a few *real* bugs lurking within this list too?
~~~
For now, I am not sure how to proceed with this information. Hence this post...
- Perhaps a consistent convention for global variable names could have
prevented lots of these cases from occurring.
- Many of these shadow cases look unintentional to me; I feel the code
would have been implemented differently had the developer been aware
of them, so at least advertising their presence seems a useful thing
to know. Perhaps the -Wshadow flag can be added to one of the
build-farm machines for that purpose?
- Finally, IMO the code is nearly always more confusing when there is
variable shadowing, so removal of these warnings seems a worthy goal.
Perhaps they can be slowly whittled away during the course of PG 15
development?
Or am I just jumping at shadows?
Thoughts?
----------
[1] https://github.com/postgres/postgres/commit/4e8c0f1a0d0d095a749a329a216c88a340a455b6
[2] https://github.com/postgres/postgres/commit/db16c656478b815627a03bb0a31833391a733eb0
[3] https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
Kind Regards,
Peter Smith.
Fujitsu Australia
