On Fri, 14 May 2021 at 12:00, Peter Smith <smithpb2...@gmail.com> wrote: > 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).
> For now, I am not sure how to proceed with this information. Hence this > post... I'm inclined to think that since a bug has already been found due to a local variable shadowing a global one that it would be good to review these and then consider if it's worth doing any renaming. I think the process of looking at each warning individually will allow us to determine if; a) there are any bugs, or; b) if it's worth doing any renaming. I see GCC also has -Wshadow=compatible-local to warn when there shadowing is going on in local vars where both vars have compatible types. -Wshadow=local is any local var shadowing, then the option you used which is the same as -Wshadow=global. I'd say it might be worth aspiring to reduce the warnings from building with these flags. If we reduced these down then it might allow us to more easily identify cases where there are actual bugs. Maybe we can get to a point where we could enable either -Wshadow=compatible-local or -Wshadow=local. I doubt we could ever get to a stage where -Wshadow=global would work for us. There's also some quite old discussion in [1] that you might want to review. I don't pretend to have found the best example of ones that we might want to leave alone, but: pg_controldata.c: In function ‘wal_level_str’: pg_controldata.c:73:24: warning: declaration of ‘wal_level’ shadows a global declaration [-Wshadow] wal_level_str(WalLevel wal_level) ^ In file included from pg_controldata.c:24:0: ../../../src/include/access/xlog.h:187:24: warning: shadowed declaration is here [-Wshadow] extern PGDLLIMPORT int wal_level; I wonder if it would really clear up much if the parameter name there was renamed not to shadow the GUC variable's name. Also, doing any renaming here is not without risk that we break something, so certainly PG15 at the earliest, unless there is an actual bug. I imagine starting with a patch that fixes the ones where the name does not have much meaning. e.g, i, buf, tmp, lc We also need to take into account that renaming variables here can increase the overhead of backpatching fixes. The process of fixing those up to make the patch apply to the back branch does increase the chances that bugs could make their way into the back branches. However, it's probably more likely to end up as a bug if the patch was written for the back branch then there's a bigger opportunity for the patch author to pick the wrong variable name when converting the patch to work with master. In the reverse case, that does not seem as likely due to both variables having the same name. David [1] https://www.postgresql.org/message-id/flat/877k1psmpf.fsf%40mailbox.samurai.com