Thread: shadow variables - pg15 edition
There's been no progress on this in the past discussions. https://www.postgresql.org/message-id/flat/877k1psmpf.fsf%40mailbox.samurai.com https://www.postgresql.org/message-id/flat/CAApHDvpqBR7u9yzW4yggjG%3DQfN%3DFZsc8Wo2ckokpQtif-%2BiQ2A%40mail.gmail.com#2d900bfe18fce17f97ec1f00800c8e27 https://www.postgresql.org/message-id/flat/MN2PR18MB2927F7B5F690065E1194B258E35D0%40MN2PR18MB2927.namprd18.prod.outlook.com But an unfortunate consequence of not fixing the historic issues is that it precludes the possibility that anyone could be expected to notice if they introduce more instances of the same problem (as in the first half of these patches). Then the hole which has already been dug becomes deeper, further increasing the burden of fixing the historic issues before being able to use -Wshadow. The first half of the patches fix shadow variables newly-introduced in v15 (including one of my own patches), the rest are fixing the lowest hanging fruit of the "short list" from COPT=-Wshadow=compatible-local I can't see that any of these are bugs, but it seems like a good goal to move towards allowing use of the -Wshadow* options to help avoid future errors, as well as cleanliness and readability (rather than allowing it to get harder to use -Wshadow). -- Justin
Attachment
- 0001-avoid-shadow-vars-pg_dump.c-i_oid.patch
- 0002-avoid-shadow-vars-pg_dump.c-tbinfo.patch
- 0003-avoid-shadow-vars-pg_dump.c-owning_tab.patch
- 0004-avoid-shadow-vars-tablesync.c-first.patch
- 0005-avoid-shadow-vars-tablesync.c-slot.patch
- 0006-avoid-shadow-vars-basebackup_target.c-ttype.patch
- 0007-avoid-shadow-vars-parse_jsontable.c-jtc.patch
- 0008-avoid-shadow-vars-res.patch
- 0009-avoid-shadow-vars-clauses.c-querytree_list.patch
- 0010-avoid-shadow-vars-tablecmds.c-constraintOid.patch
- 0011-avoid-shadow-vars-tablecmds.c-copyTuple.patch
- 0012-avoid-shadow-vars-copyfrom.c-attnum.patch
- 0013-avoid-shadow-vars-nodeAgg-transno.patch
- 0014-avoid-shadow-vars-trigger.c-partitionId.patch
- 0015-avoid-shadow-vars-execPartition.c-found_whole_row.patch
- 0016-avoid-shadow-vars-brin-keyno.patch
- 0017-avoid-shadow-vars-bufmgr.c-j.patch
On Thu, Aug 18, 2022 at 12:54 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > > There's been no progress on this in the past discussions. > > https://www.postgresql.org/message-id/flat/877k1psmpf.fsf%40mailbox.samurai.com > https://www.postgresql.org/message-id/flat/CAApHDvpqBR7u9yzW4yggjG%3DQfN%3DFZsc8Wo2ckokpQtif-%2BiQ2A%40mail.gmail.com#2d900bfe18fce17f97ec1f00800c8e27 > https://www.postgresql.org/message-id/flat/MN2PR18MB2927F7B5F690065E1194B258E35D0%40MN2PR18MB2927.namprd18.prod.outlook.com > > But an unfortunate consequence of not fixing the historic issues is that it > precludes the possibility that anyone could be expected to notice if they > introduce more instances of the same problem (as in the first half of these > patches). Then the hole which has already been dug becomes deeper, further > increasing the burden of fixing the historic issues before being able to use > -Wshadow. > > The first half of the patches fix shadow variables newly-introduced in v15 > (including one of my own patches), the rest are fixing the lowest hanging fruit > of the "short list" from COPT=-Wshadow=compatible-local > > I can't see that any of these are bugs, but it seems like a good goal to move > towards allowing use of the -Wshadow* options to help avoid future errors, as > well as cleanliness and readability (rather than allowing it to get harder to > use -Wshadow). > Hey, thanks for picking this up! I'd started looking at these [1] last year and spent a day trying to categorise them all in a spreadsheet (shadows a global, shadows a parameter, shadows a local var etc) but I became swamped by the volume, and then other work/life got in the way. +1 from me. ------ [1] https://www.postgresql.org/message-id/flat/CAHut%2BPuv4LaQKVQSErtV_%3D3MezUdpipVOMt7tJ3fXHxt_YK-Zw%40mail.gmail.com Kind Regards, Peter Smith. Fujitsu Australia
On Thu, Aug 18, 2022 at 08:49:14AM +1000, Peter Smith wrote: > I'd started looking at these [1] last year and spent a day trying to > categorise them all in a spreadsheet (shadows a global, shadows a > parameter, shadows a local var etc) but I became swamped by the > volume, and then other work/life got in the way. > > +1 from me. A lot of the changes proposed here update the code so as the same variable gets used across more code paths by removing declarations, but we have two variables defined because both are aimed to be used in a different context (see AttachPartitionEnsureIndexes() in tablecmds.c for example). Wouldn't it be a saner approach in a lot of cases to rename the shadowed variables (aka the ones getting removed in your patches) and keep them local to the code paths where we use them? -- Michael
Attachment
On Thu, Aug 18, 2022 at 09:39:02AM +0900, Michael Paquier wrote: > On Thu, Aug 18, 2022 at 08:49:14AM +1000, Peter Smith wrote: > > I'd started looking at these [1] last year and spent a day trying to > > categorise them all in a spreadsheet (shadows a global, shadows a > > parameter, shadows a local var etc) but I became swamped by the > > volume, and then other work/life got in the way. > > > > +1 from me. > > A lot of the changes proposed here update the code so as the same > variable gets used across more code paths by removing declarations, > but we have two variables defined because both are aimed to be used in > a different context (see AttachPartitionEnsureIndexes() in tablecmds.c > for example). > Wouldn't it be a saner approach in a lot of cases to rename the > shadowed variables (aka the ones getting removed in your patches) and > keep them local to the code paths where we use them? The cases where I removed a declaration are ones where the variable either hasn't yet been assigned in the outer scope (so it's safe to use first in the inner scope, since its value is later overwriten in the outer scope). Or it's no longer used in the outer scope, so it's safe to re-use it in the inner scope (as in AttachPartitionEnsureIndexes). Since you think it's saner, I changed to rename them. In the case of "first", the var is used in two independent loops, the same way, and re-initialized. In the case of found_whole_row, the var is ignored, as the comments say, so it would be silly to declare more vars to be additionally ignored. -- Justin PS. I hadn't sent the other patches which rename the variables, having assumed that the discussion would be bikeshedded to death and derail without having fixed the lowest hanging fruits. I'm attaching them those now to see what happens.
Attachment
- 0001-avoid-shadow-vars-pg_dump.c-i_oid.patch
- 0002-avoid-shadow-vars-pg_dump.c-tbinfo.patch
- 0003-avoid-shadow-vars-pg_dump.c-owning_tab.patch
- 0004-avoid-shadow-vars-tablesync.c-first.patch
- 0005-avoid-shadow-vars-tablesync.c-slot.patch
- 0006-avoid-shadow-vars-basebackup_target.c-ttype.patch
- 0007-avoid-shadow-vars-parse_jsontable.c-jtc.patch
- 0008-avoid-shadow-vars-res.patch
- 0009-avoid-shadow-vars-clauses.c-querytree_list.patch
- 0010-avoid-shadow-vars-tablecmds.c-constraintOid.patch
- 0011-avoid-shadow-vars-tablecmds.c-copyTuple.patch
- 0012-avoid-shadow-vars-heap.c-rel-tuple.patch
- 0013-avoid-shadow-vars-copyfrom.c-attnum.patch
- 0014-avoid-shadow-vars-nodeAgg-transno.patch
- 0015-avoid-shadow-vars-trigger.c-partitionId.patch
- 0016-avoid-shadow-vars-execPartition.c-found_whole_row.patch
- 0017-avoid-shadow-vars-brin-keyno.patch
- 0018-avoid-shadow-vars-bufmgr.c-j.patch
- 0019-avoid-shadow-vars-psql-command.c-host.patch
- 0020-avoid-shadow-vars-ruleutils-dpns.patch
- 0021-avoid-shadow-vars-costsize.c-subpath.patch
- 0022-avoid-shadow-vars-partitionfuncs.c-relid.patch
- 0023-avoid-shadow-vars-rangetypes_gist.c-range.patch
- 0024-avoid-shadow-vars-ecpglib-execute.c-len.patch
- 0025-avoid-shadow-vars-autovacuum.c-db.patch
- 0026-avoid-shadow-vars-basebackup.c-ti.patch
On Thu, 18 Aug 2022 at 02:54, Justin Pryzby <pryzby@telsasoft.com> wrote:
> The first half of the patches fix shadow variables newly-introduced in v15
> (including one of my own patches), the rest are fixing the lowest hanging fruit
> of the "short list" from COPT=-Wshadow=compatible-local
I wonder if it's better to fix the "big hitters" first. The idea
there would be to try to reduce the number of these warnings as
quickly and easily as possible. If we can get the numbers down fairly
significantly without too much effort, then that should provide us
with a bit more motivation to get rid of the remaining ones.
Here are the warnings grouped by the name of the variable:
$ make -s 2>&1 | grep "warning: declaration of" | grep -oP
"‘([_a-zA-Z]{1}[_a-zA-Z0-9]*)’" | sort | uniq -c
2 ‘aclresult’
3 ‘attnum’
1 ‘cell’
1 ‘cell__state’
2 ‘cmp’
2 ‘command’
1 ‘constraintOid’
1 ‘copyTuple’
1 ‘data’
1 ‘db’
1 ‘_do_rethrow’
1 ‘dpns’
1 ‘econtext’
1 ‘entry’
36 ‘expected’
1 ‘first’
1 ‘found_whole_row’
1 ‘host’
20 ‘i’
1 ‘iclause’
1 ‘idxs’
1 ‘i_oid’
4 ‘isnull’
1 ‘it’
2 ‘item’
1 ‘itemno’
1 ‘j’
1 ‘jtc’
1 ‘k’
1 ‘keyno’
7 ‘l’
13 ‘lc’
4 ‘lc__state’
1 ‘len’
1 ‘_local_sigjmp_buf’
1 ‘name’
2 ‘now’
1 ‘owning_tab’
1 ‘page’
1 ‘partitionId’
2 ‘path’
3 ‘proc’
1 ‘proclock’
1 ‘querytree_list’
1 ‘range’
1 ‘rel’
1 ‘relation’
1 ‘relid’
1 ‘rightop’
2 ‘rinfo’
1 ‘_save_context_stack’
1 ‘save_errno’
1 ‘_save_exception_stack’
1 ‘slot’
1 ‘sqlca’
9 ‘startelem’
1 ‘stmt_list’
2 ‘str’
1 ‘subpath’
1 ‘tbinfo’
1 ‘ti’
1 ‘transno’
1 ‘ttype’
1 ‘tuple’
5 ‘val’
1 ‘value2’
1 ‘wco’
1 ‘xid’
1 ‘xlogfname’
The top 5 by count here account for about half of the warnings, so
maybe is best to start with those? Likely the ones ending in __state
will fix themselves when you fix the variable with the same name
without that suffix.
The attached patch targets fixing the "expected" variable.
$ ./configure --prefix=/home/drowley/pg
CFLAGS="-Wshadow=compatible-local" > /dev/null
$ make clean -s
$ make -j -s 2>&1 | grep "warning: declaration of" | wc -l
153
$ make clean -s
$ patch -p1 < reduce_local_variable_shadow_warnings_in_regress.c.patch
$ make -j -s 2>&1 | grep "warning: declaration of" | wc -l
117
So 36 fewer warnings with the attached.
I'm probably not the only committer to want to run a mile when they
see someone posting 17 or 26 patches in an email. So maybe "bang for
buck" is a better method for getting the ball rolling here. As you
know, I was recently bitten by local shadows in af7d270dd, so I do
believe in the cause.
What do you think?
David
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> A lot of the changes proposed here update the code so as the same
> variable gets used across more code paths by removing declarations,
> but we have two variables defined because both are aimed to be used in
> a different context (see AttachPartitionEnsureIndexes() in tablecmds.c
> for example).
> Wouldn't it be a saner approach in a lot of cases to rename the
> shadowed variables (aka the ones getting removed in your patches) and
> keep them local to the code paths where we use them?
Yeah. I do not think a patch of this sort has any business changing
the scopes of variables. That moves it out of "cosmetic cleanup"
and into "hm, I wonder if this introduces any bugs". Most hackers
are going to decide that they have better ways to spend their time
than doing that level of analysis for a very noncritical patch.
regards, tom lane
On Thu, Aug 18, 2022 at 03:17:33PM +1200, David Rowley wrote: > I'm probably not the only committer to want to run a mile when they > see someone posting 17 or 26 patches in an email. So maybe "bang for > buck" is a better method for getting the ball rolling here. As you > know, I was recently bitten by local shadows in af7d270dd, so I do > believe in the cause. > > What do you think? You already fixed the shadow var introduced in master/pg16, and I sent patches for the shadow vars added in pg15 (marked as such and presented as 001-008), so perhaps it's okay to start with that ? BTW, one of the remaining warnings seems to be another buglet, which I'll write about at a later date. -- Justin
On Thu, 18 Aug 2022 at 17:16, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Thu, Aug 18, 2022 at 03:17:33PM +1200, David Rowley wrote:
> > I'm probably not the only committer to want to run a mile when they
> > see someone posting 17 or 26 patches in an email. So maybe "bang for
> > buck" is a better method for getting the ball rolling here. As you
> > know, I was recently bitten by local shadows in af7d270dd, so I do
> > believe in the cause.
> >
> > What do you think?
>
> You already fixed the shadow var introduced in master/pg16, and I sent patches
> for the shadow vars added in pg15 (marked as such and presented as 001-008), so
> perhaps it's okay to start with that ?
Alright, I made a pass over the 0001-0008 patches.
0001. I'd also rather see these 4 renamed:
+++ b/src/bin/pg_dump/pg_dump.c
@@ -3144,7 +3144,6 @@ dumpDatabase(Archive *fout)
PQExpBuffer loHorizonQry = createPQExpBuffer();
int i_relfrozenxid,
i_relfilenode,
- i_oid,
i_relminmxid;
Adding an extra 'i' (for inner) on the front seems fine to me.
0002. I don't really like the "my" name. I also see you've added the
word "this" to many other variables that are shadowing. It feels kinda
like you're missing a "self" and a "me" in there somewhere! :)
@@ -7080,21 +7080,21 @@ getConstraints(Archive *fout, TableInfo
tblinfo[], int numTables)
appendPQExpBufferChar(tbloids, '{');
for (int i = 0; i < numTables; i++)
{
- TableInfo *tbinfo = &tblinfo[i];
+ TableInfo *mytbinfo = &tblinfo[i];
How about just "tinfo"?
0003. The following is used for the exact same purpose as its shadowed
counterpart. I suggest just using the variable from the outer scope.
@@ -16799,21 +16799,21 @@ dumpSequence(Archive *fout, const TableInfo *tbinfo)
*/
if (OidIsValid(tbinfo->owning_tab) && !tbinfo->is_identity_sequence)
{
- TableInfo *owning_tab = findTableByOid(tbinfo->owning_tab);
+ TableInfo *this_owning_tab = findTableByOid(tbinfo->owning_tab);
0004. I would rather people used foreach_current_index(lc) > 0 to
determine when we're not doing the first iteration of a foreach loop.
I understand there are more complex cases with filtering that this
cannot be done, but these are highly simple and using
foreach_current_index() removes multiple lines of code and makes it
look nicer.
@@ -762,8 +762,8 @@ fetch_remote_table_info(char *nspname, char *relname,
TupleTableSlot *slot;
Oid attrsRow[] = {INT2VECTOROID};
StringInfoData pub_names;
- bool first = true;
+ first = true;
initStringInfo(&pub_names);
foreach(lc, MySubscription->publications)
0005. How about just "tslot". I'm not a fan of "this".
+++ b/src/backend/replication/logical/tablesync.c
@@ -759,7 +759,7 @@ fetch_remote_table_info(char *nspname, char *relname,
if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 150000)
{
WalRcvExecResult *pubres;
- TupleTableSlot *slot;
+ TupleTableSlot *thisslot;
0006. A see the outer shadowed counterpart is used to add a new backup
type. Since I'm not a fan of "this", how about the outer one gets
renamed to "newtype"?
+++ b/src/backend/backup/basebackup_target.c
@@ -73,9 +73,9 @@ BaseBackupAddTarget(char *name,
/* Search the target type list for an existing entry with this name. */
foreach(lc, BaseBackupTargetTypeList)
{
- BaseBackupTargetType *ttype = lfirst(lc);
+ BaseBackupTargetType *this_ttype = lfirst(lc);
0007. Meh, more "this". How about just "col".
+++ b/src/backend/parser/parse_jsontable.c
@@ -341,13 +341,13 @@ transformJsonTableChildPlan(JsonTableContext
*cxt, JsonTablePlan *plan,
/* transform all nested columns into cross/union join */
foreach(lc, columns)
{
- JsonTableColumn *jtc = castNode(JsonTableColumn, lfirst(lc));
+ JsonTableColumn *thisjtc = castNode(JsonTableColumn, lfirst(lc));
There's a discussion about reverting this entire patch. Not sure if
patching master and not backpatching to pg15 would be useful to the
people who may be doing that revert.
0008. Sorry, I had to change this one too. I just have an aversion to
variables named "temp" or "tmp".
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -3109,10 +3109,10 @@ JsonItemFromDatum(Datum val, Oid typid, int32
typmod, JsonbValue *res)
if (JsonContainerIsScalar(&jb->root))
{
- bool res PG_USED_FOR_ASSERTS_ONLY;
+ bool tmp PG_USED_FOR_ASSERTS_ONLY;
- res = JsonbExtractScalar(&jb->root, jbv);
- Assert(res);
+ tmp = JsonbExtractScalar(&jb->root, jbv);
+ Assert(tmp);
I've attached a patch which does things more along the lines of how I
would have done it. I don't think we should be back patching this
stuff.
Any objections to pushing this to master only?
David
Attachment
On Thu, Aug 18, 2022 at 5:27 PM David Rowley <dgrowleyml@gmail.com> wrote: > > On Thu, 18 Aug 2022 at 17:16, Justin Pryzby <pryzby@telsasoft.com> wrote: > > > > On Thu, Aug 18, 2022 at 03:17:33PM +1200, David Rowley wrote: > > > I'm probably not the only committer to want to run a mile when they > > > see someone posting 17 or 26 patches in an email. So maybe "bang for > > > buck" is a better method for getting the ball rolling here. As you > > > know, I was recently bitten by local shadows in af7d270dd, so I do > > > believe in the cause. > > > > > > What do you think? > > > > You already fixed the shadow var introduced in master/pg16, and I sent patches > > for the shadow vars added in pg15 (marked as such and presented as 001-008), so > > perhaps it's okay to start with that ? > > Alright, I made a pass over the 0001-0008 patches. > ... > > 0005. How about just "tslot". I'm not a fan of "this". > (I'm sure there are others like this; I just picked this one as an example) AFAICT the offending 'slot' really should have never been declared at all at the local scope in the first place - e.g. the other code in this function seems happy enough with the pattern of just re-using the function scoped 'slot'. I understand that for this shadow patch changing the var-name is considered the saner/safer way than tampering with the scope, but perhaps it is still useful to include a comment when changing ones like this? e.g. + TupleTableSlot *tslot; /* TODO - Why declare this at all? Shouldn't it just re-use the 'slot' at function scope? */ Otherwise, such knowledge will be lost, and nobody will ever know to revisit them, which feels a bit more like *hiding* the mistake than fixing it. ------ Kind Regards, Peter Smith. Fujitsu Australia
On Thu, Aug 18, 2022 at 07:27:09PM +1200, David Rowley wrote: > 0001. I'd also rather see these 4 renamed: .. > 0002. I don't really like the "my" name. I also see you've added the .. > How about just "tinfo"? .. > 0005. How about just "tslot". I'm not a fan of "this". .. > Since I'm not a fan of "this", how about the outer one gets renamed .. > 0007. Meh, more "this". How about just "col". .. > 0008. Sorry, I had to change this one too. I agree that ii_oid and newtype are better names (although it's a bit unfortunate to rename the outer "ttype" var of wider scope). > 0003. The following is used for the exact same purpose as its shadowed > counterpart. I suggest just using the variable from the outer scope. And that's what my original patch did, before people insisted that the patches shouldn't change variable scope. Now it's back to where I stared. > There's a discussion about reverting this entire patch. Not sure if > patching master and not backpatching to pg15 would be useful to the > people who may be doing that revert. I think if it were reverted, it'd be in both branches. > I've attached a patch which does things more along the lines of how I > would have done it. I don't think we should be back patching this > stuff. > > Any objections to pushing this to master only? I won't object, but some of your changes are what makes backpatching this less reasonable (foreach_current_index and newtype). I had made these v15 patches first to simplify backpatching, since having the same code in v15 means that there's no backpatch hazard for this new-in-v15 code. I am opened to presenting the patches differently, but we need to come up with a better process than one person writing patches and someone else rewriting it. I also don't see the value of debating which order to write the patches in. Grouping by variable name or doing other statistical analysis doesn't change the fact that there are 50+ issues to address to allow -Wshadow to be usable. Maybe these would be helpful ? - if I publish the patches on github; - if I send the patches with more context; - if you have an suggestion/objection/complaint with a patch, I can address it and/or re-arrange the patchset so this is later, and all the polished patches are presented first. -- Justin
On Fri, Aug 19, 2022 at 9:21 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Thu, Aug 18, 2022 at 07:27:09PM +1200, David Rowley wrote: > > 0001. I'd also rather see these 4 renamed: > .. > > 0002. I don't really like the "my" name. I also see you've added the > .. > > How about just "tinfo"? > .. > > 0005. How about just "tslot". I'm not a fan of "this". > .. > > Since I'm not a fan of "this", how about the outer one gets renamed > .. > > 0007. Meh, more "this". How about just "col". > .. > > 0008. Sorry, I had to change this one too. > > I agree that ii_oid and newtype are better names (although it's a bit > unfortunate to rename the outer "ttype" var of wider scope). > > > 0003. The following is used for the exact same purpose as its shadowed > > counterpart. I suggest just using the variable from the outer scope. > > And that's what my original patch did, before people insisted that the patches > shouldn't change variable scope. Now it's back to where I stared. > > > There's a discussion about reverting this entire patch. Not sure if > > patching master and not backpatching to pg15 would be useful to the > > people who may be doing that revert. > > I think if it were reverted, it'd be in both branches. > > > I've attached a patch which does things more along the lines of how I > > would have done it. I don't think we should be back patching this > > stuff. > > > > Any objections to pushing this to master only? > > I won't object, but some of your changes are what makes backpatching this less > reasonable (foreach_current_index and newtype). I had made these v15 patches > first to simplify backpatching, since having the same code in v15 means that > there's no backpatch hazard for this new-in-v15 code. > > I am opened to presenting the patches differently, but we need to come up with > a better process than one person writing patches and someone else rewriting it. > I also don't see the value of debating which order to write the patches in. > Grouping by variable name or doing other statistical analysis doesn't change > the fact that there are 50+ issues to address to allow -Wshadow to be usable. > > Maybe these would be helpful ? > - if I publish the patches on github; > - if I send the patches with more context; > - if you have an suggestion/objection/complaint with a patch, I can address it > and/or re-arrange the patchset so this is later, and all the polished > patches are presented first. > Starting off with patches might come to grief, and it won't be much fun rearranging patches over and over. Because there are so many changes, I think it would be better to attack this task methodically: STEP 1 - Capture every shadow warning and categorise exactly what kind is it. e.g maybe do this as some XLS which can be shared. The last time I looked there were hundreds of instances, but I expect there will be less than a couple of dozen different *categories* of them. e.g. shadow of a global var e.g. shadow of a function param e.g. shadow of a function var in a code block for the exact same usage e.g. shadow of a function var in a code block for some 'tmp' var e.g. shadow of a function var in a code block due to a mistake e.g. shadow of a function var by some loop index e.g. shadow of a function var for some loop 'first' handling e.g. bug etc... STEP 2 - Define your rules for how intend to address each of these kinds of shadows (e.g. just simple rename of the var, use 'foreach_current_index', ...). Hopefully, it will be easy to reach an agreement now since all instances of the same kind will look pretty much the same. STEP 3 - Fix all of the same kinds of shadows per single patch (using the already agreed fix approach from step 2). REPEAT STEPS 2,3 until done. ------ Kind Regards, Peter Smith. Fujitsu Australia
On Fri, 19 Aug 2022 at 11:21, Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Thu, Aug 18, 2022 at 07:27:09PM +1200, David Rowley wrote: > > Any objections to pushing this to master only? > > I won't object, but some of your changes are what makes backpatching this less > reasonable (foreach_current_index and newtype). I had made these v15 patches > first to simplify backpatching, since having the same code in v15 means that > there's no backpatch hazard for this new-in-v15 code. I spent a bit more time on this and I see that make check-world does fail if I change either of the foreach_current_index() changes to be incorrect. e.g change the condition from "> 0" to be "== 0", "> 1" or "> -1". As for the newtype change, I was inclined to give the variable name with the most meaning to the one that's in scope for longer. I'm starting to feel like it would be ok to backpatch these new-to-pg-15 changes back into PG15. The reason I think this is that they all seem low enough risk that it's probably more risky to not backpatch and risk bugs being introduced due to mistakes being made in conflict resolution when future patches don't apply. It was the failing tests I mentioned above that swayed me on this. > I am opened to presenting the patches differently, but we need to come up with > a better process than one person writing patches and someone else rewriting it. It wasn't my intention to purposefully rewrite everything. It's just that in order to get the work into something I was willing to commit, that's how it ended up. As for why I did that rather than ask you to was the fact that doing it myself required fewer keystrokes, mental effort and time than asking you to. It's not my intention to do that for any personal credit. I'm happy for you to take that. I'd just rather not be batting such trivial patches over the fence at each other for days or weeks. The effort-to-reward ratio for that is probably going to drop below my threshold after a few rounds. David
On Fri, Aug 19, 2022 at 03:37:52PM +1200, David Rowley wrote: > I'm happy for you to take that. I'd just rather not be batting such trivial > patches over the fence at each other for days or weeks. Yes, thanks for that. I read through your patch, which looks fine. Let me know what I can do when it's time for round two. -- Justin
On Fri, 19 Aug 2022 at 16:28, Justin Pryzby <pryzby@telsasoft.com> wrote: > Let me know what I can do when it's time for round two. I pushed the modified 0001-0008 patches earlier today and also the one I wrote to fixup the 36 warnings about "expected" being shadowed. I looked through a bunch of your remaining patches and was a bit unexcited to see many more renaming such as: - List *querytree_list; + List *this_querytree_list; I don't think this sort of thing is an improvement. However, one category of these changes that I do like are the ones where we can move the variable into an inner scope. Out of your renaming 0009-0026 patches, these are: 0013 0014 0017 0018 I feel like having the variable in scope for the minimal amount of time makes the code cleaner and I feel like these are good next steps because: a) no variable needs to be renamed b) any backpatching issues is more likely to lead to compilation failure rather than using the wrong variable. Likely 0016 is a subcategory of the above as if you modified that patch to follow this rule then you'd have to declare the variable a few times. I think that category is less interesting and we can maybe consider those after we're done with the more simple ones. Do you want to submit a series of patches that fixes all of the remaining warnings that are in this category? Once these are done we can consider the best ways to fix and if we want to fix any of the remaining ones. Feel free to gzip the patches up if the number is large. David
On Sat, Aug 20, 2022 at 09:17:41PM +1200, David Rowley wrote: > On Fri, 19 Aug 2022 at 16:28, Justin Pryzby <pryzby@telsasoft.com> wrote: > > Let me know what I can do when it's time for round two. > > I pushed the modified 0001-0008 patches earlier today and also the one > I wrote to fixup the 36 warnings about "expected" being shadowed. Thank you > I looked through a bunch of your remaining patches and was a bit > unexcited to see many more renaming such as: Yes - after Michael said that was the sane procedure, I had rearranged the patch series to present eariler those patches first which renamed variables .. > However, one category of these changes that I do like are the ones > where we can move the variable into an inner scope. There are a lot of these, which ISTM is a good thing. This fixes about half of the remaining warnings. https://github.com/justinpryzby/postgres/tree/avoid-shadow-vars You could review without applying the patches, on the webpage or (probably better) by adding as a git remote. Attached is a squished version. -- Justin
Attachment
On Tue, 23 Aug 2022 at 13:17, Justin Pryzby <pryzby@telsasoft.com> wrote:
> Attached is a squished version.
I see there's some renaming ones snuck in there. e.g:
- Relation rel;
- HeapTuple tuple;
+ Relation pg_foreign_table;
+ HeapTuple foreigntuple;
This one does not seem to be in the category I mentioned:
@@ -3036,8 +3036,6 @@ XLogFileInitInternal(XLogSegNo logsegno,
TimeLineID logtli,
pgstat_report_wait_start(WAIT_EVENT_WAL_INIT_SYNC);
if (pg_fsync(fd) != 0)
{
- int save_errno = errno;
-
More renaming:
+++ b/src/backend/catalog/heap.c
@@ -1818,19 +1818,19 @@ heap_drop_with_catalog(Oid relid)
*/
if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
{
- Relation rel;
- HeapTuple tuple;
+ Relation pg_foreign_table;
+ HeapTuple foreigntuple;
More renaming:
+++ b/src/backend/commands/publicationcmds.c
@@ -106,7 +106,7 @@ parse_publication_options(ParseState *pstate,
{
char *publish;
List *publish_list;
- ListCell *lc;
+ ListCell *lc2;
and again:
+++ b/src/backend/commands/tablecmds.c
@@ -10223,7 +10223,7 @@ CloneFkReferencing(List **wqueue, Relation
parentRel, Relation partRel)
Oid constrOid;
ObjectAddress address,
referenced;
- ListCell *cell;
+ ListCell *lc;
I've not checked the context one this, but this does not appear to
meet the category of moving to an inner scope:
+++ b/src/backend/executor/execPartition.c
@@ -768,7 +768,6 @@ ExecInitPartitionInfo(ModifyTableState *mtstate,
EState *estate,
{
List *onconflset;
List *onconflcols;
- bool found_whole_row;
Looks like you're just using the one from the wider scope. That's not
the category we're after for now.
You've also got some renaming going on in ExecInitAgg()
- phasedata->gset_lengths[i] = perhash->numCols = aggnode->numCols;
+ phasedata->gset_lengths[setno] = perhash->numCols = aggnode->numCols;
I wondered about this one too:
- i = -1;
- while ((i = bms_next_member(all_grouped_cols, i)) >= 0)
- aggstate->all_grouped_cols = lcons_int(i, aggstate->all_grouped_cols);
+ {
+ int i = -1;
+ while ((i = bms_next_member(all_grouped_cols, i)) >= 0)
+ aggstate->all_grouped_cols = lcons_int(i, aggstate->all_grouped_cols);
+ }
I had in mind that maybe we should switch those to be something more like:
for (int i = -1; (i = bms_next_member(all_grouped_cols, i)) >= 0;)
But I had 2nd thoughts as the "while" version has become the standard method.
(Really that code should be using bms_prev_member() and lappend_int()
so we don't have to memmove() the entire list each lcons_int() call.
(not for this patch though))
More renaming being done here:
- int i; /* Index into *ident_user */
+ int j; /* Index into *ident_user */
... in fact, there's lots of renaming, so I'll just stop looking.
Can you just send a patch that only changes the cases where you can
remove a variable declaration from an outer scope into a single inner
scope, or multiple inner scope when the variable can be declared
inside a for() loop? The mcv_get_match_bitmap() change is an example
of this. There's still a net reduction in lines of code, so I think
the mcv_get_match_bitmap(), and any like it are ok for this next step.
A counter example is ExecInitPartitionInfo() where the way to do this
would be to move the found_whole_row declaration into multiple inner
scopes. That's a net increase in code lines, for which I think
requires more careful thought if we want that or not.
David
On Tue, Aug 23, 2022 at 01:38:40PM +1200, David Rowley wrote: > On Tue, 23 Aug 2022 at 13:17, Justin Pryzby <pryzby@telsasoft.com> wrote: > > Attached is a squished version. > > I see there's some renaming ones snuck in there. e.g: > ... in fact, there's lots of renaming, so I'll just stop looking. Actually, they didn't sneak in - what I sent are the patches which are ready to be reviewed, excluding the set of "this" and "tmp" and other renames which you disliked. In the branch (not the squished patch) the first ~15 patches were mostly for C99 for loops - I presented them this way deliberately, so you could review and comment on whatever you're able to bite off, or run with whatever parts you think are ready. I rewrote it now to be more bite sized by truncating off the 2nd half of the patches. > Can you just send a patch that only changes the cases where you can > remove a variable declaration from an outer scope into a single inner > scope, or multiple inner scope when the variable can be declared > inside a for() loop? > would be to move the found_whole_row declaration into multiple inner > scopes. That's a net increase in code lines, for which I think > requires more careful thought if we want that or not. IMO it doesn't make sense to declare multiple integers for something like this whether they're all ignored. Nor for "save_errno" nor the third, similar case, for the reason in the commit message. -- Justin
Attachment
On Tue, 23 Aug 2022 at 14:14, Justin Pryzby <pryzby@telsasoft.com> wrote:
> Actually, they didn't sneak in - what I sent are the patches which are ready to
> be reviewed, excluding the set of "this" and "tmp" and other renames which you
> disliked. In the branch (not the squished patch) the first ~15 patches were
> mostly for C99 for loops - I presented them this way deliberately, so you could
> review and comment on whatever you're able to bite off, or run with whatever
> parts you think are ready. I rewrote it now to be more bite sized by
> truncating off the 2nd half of the patches.
Thanks for the updated patch.
I've now pushed it after making some small adjustments.
It seems there was one leftover rename still there, I removed that.
The only other changes I made were to just make the patch mode
consistent with what it was doing. There were a few cases where you
were doing:
if (typlen == -1) /* varlena */
{
- int i;
-
- for (i = 0; i < nvalues; i++)
+ for (int i = 0; i < nvalues; i++)
That wasn't really required to remove the warning as you'd already
adjusted the scope of the shadowed variable so there was no longer a
collision. The reason I adjusted these was because sometimes you were
doing that, and sometimes you were not. I wanted to be consistent, so
I opted for not doing it as it's not required for this effort. Maybe
one day those can be changed in some other unrelated effort to C99ify
our code.
The attached patch is just the portions I didn't commit.
Thanks for working on this.
David
Attachment
On Wed, Aug 24, 2022 at 12:37:29PM +1200, David Rowley wrote: > On Tue, 23 Aug 2022 at 14:14, Justin Pryzby <pryzby@telsasoft.com> wrote: > > Actually, they didn't sneak in - what I sent are the patches which are ready to > > be reviewed, excluding the set of "this" and "tmp" and other renames which you > > disliked. In the branch (not the squished patch) the first ~15 patches were > > mostly for C99 for loops - I presented them this way deliberately, so you could > > review and comment on whatever you're able to bite off, or run with whatever > > parts you think are ready. I rewrote it now to be more bite sized by > > truncating off the 2nd half of the patches. > > Thanks for the updated patch. > > I've now pushed it after making some small adjustments. Thanks for handling them. Attached are half of the remainder of what I've written, ready for review. I also put it here: https://github.com/justinpryzby/postgres/tree/avoid-shadow-vars You may or may not find the associated commit messages to be useful. Let me know if you'd like the individual patches included here, instead. The first patch removes 2ndary, "inner" declarations, where that seems reasonably safe and consistent with existing practice (and probably what the original authors intended or would have written). -- Justin
Attachment
On Wed, 24 Aug 2022 at 14:39, Justin Pryzby <pryzby@telsasoft.com> wrote: > Attached are half of the remainder of what I've written, ready for review. Thanks for the patches. I started to do some analysis of the remaining warnings and put them in the attached spreadsheet. I put each of the remaining warnings into a category of how I think they should be fixed. These categories are: 1. "Rescope" (adjust scope of outer variable to move it into a deeper scope) 2. "Rename" (a variable needs to be renamed) 3. "RenameOrScope" (a variable needs renamed or we need to something more extreme to rescope) 4. "Repurpose" (variables have the same purpose and may as well use the same variable) 5. "Refactor" (fix the code to make it better) 6. "Remove" (variable is not needed) There's also: 7. "Bug?" (might be a bug) 8. "?" (I don't know) I was hoping we'd already caught all of the #1s in 421892a19, but I caught a few of those in some of your other patches. One you'd done another way and some you'd done the rescope but just put it in the wrong patch. The others had not been done yet. I just pushed f959bf9a5 to fix those ones. I really think #2s should be done last. I'm not as comfortable with the renaming and we might want to discuss tactics on that. We could either opt to rename the shadowed or shadowing variable, or both. If we rename the shadowing variable, then pending patches or forward patches could use the wrong variable. If we rename the shadowed variable then it's not impossible that backpatching could go wrong where the new code intends to reference the outer variable using the newly named variable, but when that's backpatched it uses the variable with the same name in the inner scope. Renaming both would make the problem more obvious. I'm not sure which is best. The answer may depend on how many lines the variable is in scope for. If it's just for a few lines then the hunk context would conflict and the committer would likely notice the issue when resolving the conflict. For #3, I just couldn't decide the best fix. Many of these could be moved into an inner scope, but it would require indenting a large amount of code, e.g. in a switch() statement's "case:" to allow variables to be declared within the case. I think probably #4 should be next to do (maybe after #5) I have some ideas on how to fix the two #5s, so I'm going to go and do that now. There's only 1 #6. I'm not so sure on that yet. The variable being assigned to the variable is the current time and I'm not sure if we can reuse the existing variable or not as time may have moved on sufficiently. I'll study #7 a bit more. My eyes glazed over a bit from doing all that analysis, so I might be mistaken about that being a bug. For #8s. These are the PG_TRY() ones. I see you had a go at fixing that by moving the nested PG_TRY()s to a helper function. I don't think that's a good fix. If we were to ever consider making -Wshadow=compatible-local in a standard build, then we'd basically be saying that nested PG_TRYs are not allowed. I don't think that'll fly. I'd rather find a better way to fix those. I see we can't make use of ##__LINE__ in the variable name since PG_TRY()'s friends use the variables too and they'd be on a different line. We maybe could have an "ident" parameter in the macro that we ##ident onto the variables names, but that would break existing code. > The first patch removes 2ndary, "inner" declarations, where that seems > reasonably safe and consistent with existing practice (and probably what the > original authors intended or would have written). Would you be able to write a patch for #4. I'll do #5 now. You could do a draft patch for #2 as well, but I think it should be committed last, if we decide it's a good move to make. It may be worth having the discussion about if we actually want to run -Wshadow=compatible-local as a standard build flag before we rename anything. David
Attachment
On Wed, Aug 24, 2022 at 10:47:31PM +1200, David Rowley wrote: > I was hoping we'd already caught all of the #1s in 421892a19, but I > caught a few of those in some of your other patches. One you'd done > another way and some you'd done the rescope but just put it in the > wrong patch. The others had not been done yet. I just pushed > f959bf9a5 to fix those ones. This fixed pg_get_statisticsobj_worker() but not pg_get_indexdef_worker() nor pg_get_partkeydef_worker(). (Also, I'd mentioned that my fixes for those deliberately re-used the outer-scope vars, which isn't what you did, and it's why I didn't include them with the patch for inner-scope). > I really think #2s should be done last. I'm not as comfortable with > the renaming and we might want to discuss tactics on that. We could > either opt to rename the shadowed or shadowing variable, or both. If > we rename the shadowing variable, then pending patches or forward > patches could use the wrong variable. If we rename the shadowed > variable then it's not impossible that backpatching could go wrong > where the new code intends to reference the outer variable using the > newly named variable, but when that's backpatched it uses the variable > with the same name in the inner scope. Renaming both would make the > problem more obvious. I'm not sure which is best. The answer may > depend on how many lines the variable is in scope for. If it's just > for a few lines then the hunk context would conflict and the committer > would likely notice the issue when resolving the conflict. Yes, the hope is to limit the change to variables that are only used a couple times within a few lines. It's also possible that these will break patches in development, but that's normal for any change at all. > I'll study #7 a bit more. My eyes glazed over a bit from doing all > that analysis, so I might be mistaken about that being a bug. I reported this last week. https://www.postgresql.org/message-id/20220819211824.GX26426@telsasoft.com -- Justin
On Thu, 25 Aug 2022 at 02:00, Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Wed, Aug 24, 2022 at 10:47:31PM +1200, David Rowley wrote: > > I was hoping we'd already caught all of the #1s in 421892a19, but I > > caught a few of those in some of your other patches. One you'd done > > another way and some you'd done the rescope but just put it in the > > wrong patch. The others had not been done yet. I just pushed > > f959bf9a5 to fix those ones. > > This fixed pg_get_statisticsobj_worker() but not pg_get_indexdef_worker() nor > pg_get_partkeydef_worker(). The latter two can't be fixed in the same way as pg_get_statisticsobj_worker(), which is why I left them alone. We can deal with those when getting onto the next category of warnings, which I believe should be the "Repurpose" category. If you look at the shadow_analysis spreadsheet then you can see how I've categorised each. I'm not pretending those are all 100% accurate. Various cases the choice of category was subjective. My aim here is to fix as many of the warnings as possible in the safest way possible for the particular warning. This is why pg_get_statisticsobj_worker() wasn't fixed in the same pass as pg_get_indexdef_worker() and pg_get_partkeydef_worker(). David
On Wed, 24 Aug 2022 at 22:47, David Rowley <dgrowleyml@gmail.com> wrote: > 5. "Refactor" (fix the code to make it better) > I have some ideas on how to fix the two #5s, so I'm going to go and do that now. I've attached a patch which I think improves the code in gistRelocateBuildBuffersOnSplit() so that there's no longer a shadowed variable. I also benchmarked this method in a tight loop and can measure no performance change from getting the loop index this way vs the old way. This only fixes one of the #5s I mentioned. I ended up scraping my idea to fix the shadowed 'i' in get_qual_for_range() as it became too complex. The idea was to use list_cell_number() to find out how far we looped in the forboth() loop. It turned out that 'i' was used in the subsequent loop in "j = i;". The fix just became too complex and I didn't think it was worth the risk of breaking something just to get rid of the showed 'i'. David
Attachment
On Wed, Aug 24, 2022 at 10:47:31PM +1200, David Rowley wrote: > On Wed, 24 Aug 2022 at 14:39, Justin Pryzby <pryzby@telsasoft.com> wrote: > > Attached are half of the remainder of what I've written, ready for review. > > Thanks for the patches. > 4. "Repurpose" (variables have the same purpose and may as well use > the same variable) > Would you be able to write a patch for #4. The first of the patches that I sent yesterday was all about "repurposed" vars from outer scope (lc, l, isnull, save_errno), and was 70% of your list of vars to repurpose. Here, I've included the rest of your list. Plus another patch for vars which I'd already written patches to repurpose, but which aren't classified as "repurpose" on your list. For subselect.c, you could remove some more "lc" vars and re-use the "l" var for consistency (but I suppose you won't want that). -- Justin
Attachment
On Thu, 25 Aug 2022 at 14:08, Justin Pryzby <pryzby@telsasoft.com> wrote: > Here, I've included the rest of your list. OK, I've gone through v3-remove-var-declarations.txt, v4-reuse.txt v4-reuse-more.txt and committed most of what you had and removed a few that I thought should be renames instead. I also added some additional ones after reprocessing the RenameOrScope category from the spreadsheet. With some minor adjustments to a small number of your ones, I pushed what I came up with. David
Attachment
On Thu, 25 Aug 2022 at 13:46, David Rowley <dgrowleyml@gmail.com> wrote: > I've attached a patch which I think improves the code in > gistRelocateBuildBuffersOnSplit() so that there's no longer a shadowed > variable. I also benchmarked this method in a tight loop and can > measure no performance change from getting the loop index this way vs > the old way. I've now pushed this patch too. David
On Wed, Aug 24, 2022 at 10:47:31PM +1200, David Rowley wrote: > I really think #2s should be done last. I'm not as comfortable with > the renaming and we might want to discuss tactics on that. We could > either opt to rename the shadowed or shadowing variable, or both. If > we rename the shadowing variable, then pending patches or forward > patches could use the wrong variable. If we rename the shadowed > variable then it's not impossible that backpatching could go wrong > where the new code intends to reference the outer variable using the > newly named variable, but when that's backpatched it uses the variable > with the same name in the inner scope. Renaming both would make the > problem more obvious. The most *likely* outcome of renaming the *outer* variable is that *every* cherry-pick involving that variable would fails to compile, which is an *obvious* failure (good) but also kind of annoying if it could've worked fine if it weren't renamed. I think most of the renames should be applied to the inner var, because it's of narrower scope, and more likely to cause a conflict (good) rather than appearing to apply cleanly but then misbehave. But it seems reasonable to consider renaming both if the inner scope is longer than a handful of lines. > Would you be able to write a patch for #4. I'll do #5 now. You could > do a draft patch for #2 as well, but I think it should be committed > last, if we decide it's a good move to make. It may be worth having > the discussion about if we actually want to run > -Wshadow=compatible-local as a standard build flag before we rename > anything. I'm afraid the discussion about default flags would distract from fixing the individual warnings, which itself preclude usability of the flag by individual developers, or buildfarm, even as a local setting. It can't be enabled until *all* the shadows are gone, due to -Werror on the buildfarm and cirrusci. Unless perhaps we used -Wno-error=shadow. I suppose we're only talking about enabling it for gcc? The biggest benefit is if we fix *all* the local shadow vars, since that allows someone to make use of the option, and thereby avoiding future such issues. Enabling the option could conceivably avoid issues cherry-picking into back branch - if an inner var is re-introduced during conflict resolution, then a new warning would be issued, and hopefully the developer would look more closely. Would you check if any of these changes are good enough ? -- Justin
Attachment
On Tue, 30 Aug 2022 at 17:44, Justin Pryzby <pryzby@telsasoft.com> wrote: > Would you check if any of these changes are good enough ? I looked through v5.txt and modified it so that the fix for the shadow warnings are more aligned to the spreadsheet I created. I also fixed some additional warnings which leaves just 5 warnings. Namely: ../../../src/include/utils/elog.h:317:29: warning: declaration of ‘_save_exception_stack’ shadows a previous local ../../../src/include/utils/elog.h:318:39: warning: declaration of ‘_save_context_stack’ shadows a previous local ../../../src/include/utils/elog.h:319:28: warning: declaration of ‘_local_sigjmp_buf’ shadows a previous local ../../../src/include/utils/elog.h:320:22: warning: declaration of ‘_do_rethrow’ shadows a previous local pgbench.c:7509:40: warning: declaration of ‘now’ shadows a previous local The first 4 of those are due to a nested PG_TRY(). The final one I just ran out of inspiration on what to rename the variable to. If there are no objections then I'll push this in the next day or 2. David
Attachment
On Tue, Oct 04, 2022 at 02:27:09PM +1300, David Rowley wrote:
> On Tue, 30 Aug 2022 at 17:44, Justin Pryzby <pryzby@telsasoft.com> wrote:
> > Would you check if any of these changes are good enough ?
>
> I looked through v5.txt and modified it so that the fix for the shadow
> warnings are more aligned to the spreadsheet I created.
Thanks
> diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
> index 350039cc86..7848deeea9 100644
> --- a/src/backend/utils/adt/datetime.c
> +++ b/src/backend/utils/adt/datetime.c
> @@ -1019,17 +1019,17 @@ DecodeDateTime(char **field, int *ftype, int nf,
> if (ptype == DTK_JULIAN)
> {
> char *cp;
> - int val;
> + int jday;
>
> if (tzp == NULL)
> return DTERR_BAD_FORMAT;
>
> errno = 0;
> - val = strtoint(field[i], &cp, 10);
> + jday = strtoint(field[i], &cp, 10);
> if (errno == ERANGE || val < 0)
> return DTERR_FIELD_OVERFLOW;
Here, you forgot to change "val < 0".
I tried to see how to make that fail (differently) but can't see yet how
pass a negative julian date..
--
Justin
On Tue, 4 Oct 2022 at 15:30, Justin Pryzby <pryzby@telsasoft.com> wrote: > Here, you forgot to change "val < 0". Thanks. I made another review pass of each change to ensure I didn't miss any others. There were no other issues, so I pushed the adjusted patch. 5 warnings remain. 4 of these are for PG_TRY() and co. David
On Wed, 5 Oct 2022 at 21:05, David Rowley <dgrowleyml@gmail.com> wrote: > 5 warnings remain. 4 of these are for PG_TRY() and co. I've attached a draft patch for a method I was considering to fix the warnings we're getting from the nested PG_TRY() statement in utility.c. The C preprocessor does not allow name overloading in macros, but of course, it does allow variable argument marcos with ... so I just used that and added ##__VA_ARGS__ to each variable. I think that should work ok providing callers only supply 0 or 1 arguments to the macro, and of course, make that parameter value the same for each set of macros used in the PG_TRY() statement. The good thing about the optional argument is that we don't need to touch any existing users of PG_TRY(). The attached just modifies the inner-most PG_TRY() in the only nested PG_TRY() we have in the tree in utility.c. The only warning remaining after applying the attached is the "now" warning in pgbench.c:7509. I'd considered changing this to "thenow" which translates to "right now" in the part of Scotland that I'm from. I also considered "nownow", which is used in South Africa [1]. Anyway, I'm not really being serious, but I didn't come up with anything better than "now2". It's just I didn't like that as it sort of implies there are multiple definitions of "now" and I struggle with that... maybe I'm just thinking too much in terms of Newtonian Relativity... David [1] https://www.goodthingsguy.com/fun/now-now-just-now/
Attachment
On 2022-Oct-05, David Rowley wrote:
> The only warning remaining after applying the attached is the "now"
> warning in pgbench.c:7509. I'd considered changing this to "thenow"
> which translates to "right now" in the part of Scotland that I'm from.
> I also considered "nownow", which is used in South Africa [1].
> Anyway, I'm not really being serious, but I didn't come up with
> anything better than "now2". It's just I didn't like that as it sort
> of implies there are multiple definitions of "now" and I struggle with
> that... maybe I'm just thinking too much in terms of Newtonian
> Relativity...
:-D
A simpler idea might be to just remove the inner declaration, and have
that block set the outer var. There's no damage, since the block is
going to end and not access the previous value anymore.
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index aa1a3541fe..91a067859b 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -7506,7 +7506,7 @@ threadRun(void *arg)
/* progress report is made by thread 0 for all threads */
if (progress && thread->tid == 0)
{
- pg_time_usec_t now = pg_time_now();
+ now = pg_time_now(); /* not lazy; clobbers outer value */
if (now >= next_report)
{
The "now now" reference reminded me of "ahorita"
https://doorwaytomexico.com/paulina/ahorita-meaning-examples/
which is source of misunderstandings across borders in South America ...
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"The important things in the world are problems with society that we don't
understand at all. The machines will become more complicated but they won't
be more complicated than the societies that run them." (Freeman Dyson)
David Rowley <dgrowleyml@gmail.com> writes:
> I've attached a draft patch for a method I was considering to fix the
> warnings we're getting from the nested PG_TRY() statement in
> utility.c.
+1
> The only warning remaining after applying the attached is the "now"
> warning in pgbench.c:7509. I'd considered changing this to "thenow"
> which translates to "right now" in the part of Scotland that I'm from.
> I also considered "nownow", which is used in South Africa [1].
> Anyway, I'm not really being serious, but I didn't come up with
> anything better than "now2".
Yeah, "now2" seems as reasonable as anything.
regards, tom lane
On Thu, 6 Oct 2022 at 03:19, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > David Rowley <dgrowleyml@gmail.com> writes: > > I've attached a draft patch for a method I was considering to fix the > > warnings we're getting from the nested PG_TRY() statement in > > utility.c. > > +1 Pushed. > > The only warning remaining after applying the attached is the "now" > > warning in pgbench.c:7509. I'd considered changing this to "thenow" > > which translates to "right now" in the part of Scotland that I'm from. > > I also considered "nownow", which is used in South Africa [1]. > > Anyway, I'm not really being serious, but I didn't come up with > > anything better than "now2". > > Yeah, "now2" seems as reasonable as anything. Also pushed. (Thanks for saving me on that one.) David
Hi,
On 2022-10-06 10:21:41 +1300, David Rowley wrote:
> Also pushed. (Thanks for saving me on that one.)
Your commit message said the last shadowed variable. But building with
-Wshadow=compatible-local triggers a bunch of warnings for me (see trimmed at
the end). Looks like it "only" fixed it for src/, without optional
dependencies like gssapi and python.
I think we should add -Wshadow=compatible-local to our sets of warning flags
after fixing those.
[237/1827 42 12%] Compiling C object src/interfaces/libpq/libpq.a.p/fe-secure-gssapi.c.o
../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-secure-gssapi.c: In function ‘pg_GSS_write’:
../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-secure-gssapi.c:138:41: warning: declaration of ‘ret’
shadowsa previous local [-Wshadow=compatible-local]
138 | ssize_t ret;
| ^~~
../../../../home/andres/src/postgresql/src/interfaces/libpq/fe-secure-gssapi.c:92:25: note: shadowed declaration is
here
92 | ssize_t ret = -1;
| ^~~
[1283/1827 42 70%] Compiling C object src/pl/plpython/plpython3.so.p/plpy_cursorobject.c.o
In file included from ../../../../home/andres/src/postgresql/src/include/postgres.h:48,
from ../../../../home/andres/src/postgresql/src/pl/plpython/plpy_cursorobject.c:7:
../../../../home/andres/src/postgresql/src/pl/plpython/plpy_cursorobject.c: In function ‘PLy_cursor_plan’:
../../../../home/andres/src/postgresql/src/include/utils/elog.h:325:29: warning: declaration of ‘_save_exception_stack’
shadowsa previous local [-Wshadow=compatible-local]
325 | sigjmp_buf *_save_exception_stack##__VA_ARGS__ = PG_exception_stack; \
| ^~~~~~~~~~~~~~~~~~~~~
...
[1289/1827 42 70%] Compiling C object src/pl/plpython/plpython3.so.p/plpy_exec.c.o
../../../../home/andres/src/postgresql/src/pl/plpython/plpy_exec.c: In function ‘PLy_exec_trigger’:
../../../../home/andres/src/postgresql/src/pl/plpython/plpy_exec.c:378:46: warning: declaration of ‘tdata’ shadows a
previouslocal [-Wshadow=compatible-local]
378 | TriggerData *tdata = (TriggerData *) fcinfo->context;
| ^~~~~
../../../../home/andres/src/postgresql/src/pl/plpython/plpy_exec.c:310:22: note: shadowed declaration is here
310 | TriggerData *tdata;
| ^~~~~
[1291/1827 42 70%] Compiling C object src/pl/plpython/plpython3.so.p/plpy_spi.c.o
In file included from ../../../../home/andres/src/postgresql/src/include/postgres.h:48,
from ../../../../home/andres/src/postgresql/src/pl/plpython/plpy_spi.c:7:
../../../../home/andres/src/postgresql/src/pl/plpython/plpy_spi.c: In function ‘PLy_spi_execute_plan’:
../../../../home/andres/src/postgresql/src/include/utils/elog.h:325:29: warning: declaration of ‘_save_exception_stack’
shadowsa previous local [-Wshadow=compatible-local]
325 | sigjmp_buf *_save_exception_stack##__VA_ARGS__ = PG_exception_stack; \
| ^~~~~~~~~~~~~~~~~~~~~
[1344/1827 42 73%] Compiling C object contrib/bloom/bloom.so.p/blinsert.c.o
../../../../home/andres/src/postgresql/contrib/bloom/blinsert.c: In function ‘blinsert’:
../../../../home/andres/src/postgresql/contrib/bloom/blinsert.c:235:33: warning: declaration of ‘page’ shadows a
previouslocal [-Wshadow=compatible-local]
235 | Page page;
| ^~~~
../../../../home/andres/src/postgresql/contrib/bloom/blinsert.c:210:25: note: shadowed declaration is here
210 | Page page,
| ^~~~
[1415/1827 42 77%] Compiling C object contrib/file_fdw/file_fdw.so.p/file_fdw.c.o
../../../../home/andres/src/postgresql/contrib/file_fdw/file_fdw.c: In function ‘get_file_fdw_attribute_options’:
../../../../home/andres/src/postgresql/contrib/file_fdw/file_fdw.c:453:29: warning: declaration of ‘options’ shadows a
previouslocal [-Wshadow=compatible-local]
453 | List *options;
| ^~~~~~~
../../../../home/andres/src/postgresql/contrib/file_fdw/file_fdw.c:443:21: note: shadowed declaration is here
443 | List *options = NIL;
| ^~~~~~~
[1441/1827 42 78%] Compiling C object contrib/hstore/hstore.so.p/hstore_io.c.o
In file included from ../../../../home/andres/src/postgresql/contrib/hstore/hstore_io.c:12:
../../../../home/andres/src/postgresql/contrib/hstore/hstore_io.c: In function ‘hstorePairs’:
../../../../home/andres/src/postgresql/contrib/hstore/hstore.h:131:21: warning: declaration of ‘buflen’ shadows a
parameter[-Wshadow=compatible-local]
131 | int buflen = (ptr_) - (buf_); \
| ^~~~~~
../../../../home/andres/src/postgresql/contrib/hstore/hstore_io.c:411:9: note: in expansion of macro ‘HS_FINALIZE’
411 | HS_FINALIZE(out, pcount, buf, ptr);
| ^~~~~~~~~~~
../../../../home/andres/src/postgresql/contrib/hstore/hstore_io.c:388:47: note: shadowed declaration is here
388 | hstorePairs(Pairs *pairs, int32 pcount, int32 buflen)
| ~~~~~~^~~~~~
[1564/1827 42 85%] Compiling C object contrib/postgres_fdw/postgres_fdw.so.p/deparse.c.o
../../../../home/andres/src/postgresql/contrib/postgres_fdw/deparse.c: In function ‘foreign_expr_walker’:
../../../../home/andres/src/postgresql/contrib/postgres_fdw/deparse.c:946:53: warning: declaration of ‘lc’ shadows a
previouslocal [-Wshadow=compatible-local]
946 | ListCell *lc;
| ^~
../../../../home/andres/src/postgresql/contrib/postgres_fdw/deparse.c:904:45: note: shadowed declaration is here
904 | ListCell *lc;
| ^~
[1575/1827 38 86%] Compiling C object src/test/modules/test_integerset/test_integerset.so.p/test_integerset.c.o
../../../../home/andres/src/postgresql/src/test/modules/test_integerset/test_integerset.c: In function
‘test_huge_distances’:
../../../../home/andres/src/postgresql/src/test/modules/test_integerset/test_integerset.c:588:33: warning: declaration
of‘x’ shadows a previous local [-Wshadow=compatible-local]
588 | uint64 x = values[i];
| ^
../../../../home/andres/src/postgresql/src/test/modules/test_integerset/test_integerset.c:526:25: note: shadowed
declarationis here
526 | uint64 x;
| ^
[1633/1827 3 89%] Compiling C object contrib/postgres_fdw/postgres_fdw.so.p/postgres_fdw.c.o
../../../../home/andres/src/postgresql/contrib/postgres_fdw/postgres_fdw.c: In function ‘postgresGetForeignPlan’:
../../../../home/andres/src/postgresql/contrib/postgres_fdw/postgres_fdw.c:1344:37: warning: declaration of ‘lc’
shadowsa previous local [-Wshadow=compatible-local]
1344 | ListCell *lc;
| ^~
../../../../home/andres/src/postgresql/contrib/postgres_fdw/postgres_fdw.c:1238:21: note: shadowed declaration is here
1238 | ListCell *lc;
| ^~
Greetings,
Andres Freund
On Thu, 6 Oct 2022 at 02:34, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> A simpler idea might be to just remove the inner declaration, and have
> that block set the outer var. There's no damage, since the block is
> going to end and not access the previous value anymore.
>
> diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
> index aa1a3541fe..91a067859b 100644
> --- a/src/bin/pgbench/pgbench.c
> +++ b/src/bin/pgbench/pgbench.c
> @@ -7506,7 +7506,7 @@ threadRun(void *arg)
> /* progress report is made by thread 0 for all threads */
> if (progress && thread->tid == 0)
> {
> - pg_time_usec_t now = pg_time_now();
> + now = pg_time_now(); /* not lazy; clobbers outer value */
I didn't want to do it that way because all this code is in a while
loop and the outer "now" will be reused after it's set by the code
above. It's not really immediately obvious to me what repercussions
that would have, but it didn't seem worth taking any risks.
David
On Thu, 6 Oct 2022 at 10:40, Andres Freund <andres@anarazel.de> wrote: > Your commit message said the last shadowed variable. But building with > -Wshadow=compatible-local triggers a bunch of warnings for me (see trimmed at > the end). Looks like it "only" fixed it for src/, without optional > dependencies like gssapi and python. Well, that's embarrassing. You're right. I only fixed the ones I saw from running make in the base directory of the tree. I'll set about fixing these nownow. David
On Thu, 6 Oct 2022 at 11:50, David Rowley <dgrowleyml@gmail.com> wrote: > > On Thu, 6 Oct 2022 at 10:40, Andres Freund <andres@anarazel.de> wrote: > > Your commit message said the last shadowed variable. But building with > > -Wshadow=compatible-local triggers a bunch of warnings for me (see trimmed at > > the end). Looks like it "only" fixed it for src/, without optional > > dependencies like gssapi and python. > > Well, that's embarrassing. You're right. I only fixed the ones I saw > from running make in the base directory of the tree. I'll set about > fixing these nownow. Here's a patch which (I think) fixes the ones I missed. David
Attachment
Hi,
On 2022-10-06 13:00:41 +1300, David Rowley wrote:
> Here's a patch which (I think) fixes the ones I missed.
Yep, does the trick for me.
I attached a patch to add -Wshadow=compatible-local to our set of warnings.
> diff --git a/contrib/hstore/hstore.h b/contrib/hstore/hstore.h
> index 4713e6ea7a..897af244a4 100644
> --- a/contrib/hstore/hstore.h
> +++ b/contrib/hstore/hstore.h
> @@ -128,15 +128,15 @@ typedef struct
> /* finalize a newly-constructed hstore */
> #define HS_FINALIZE(hsp_,count_,buf_,ptr_) \
> do { \
> - int buflen = (ptr_) - (buf_); \
> + int _buflen = (ptr_) - (buf_); \
Not pretty. Given that HS_FINALIZE already has multiple-eval hazards, perhaps
we could just remove the local?
> --- a/src/interfaces/libpq/fe-secure-gssapi.c
> +++ b/src/interfaces/libpq/fe-secure-gssapi.c
> @@ -135,11 +135,11 @@ pg_GSS_write(PGconn *conn, const void *ptr, size_t len)
> */
> if (PqGSSSendLength)
> {
> - ssize_t ret;
> + ssize_t retval;
That looks like it could easily lead to confusion further down the
line. Wouldn't the better fix here be to remove the inner variable?
> --- a/src/pl/plpython/plpy_exec.c
> +++ b/src/pl/plpython/plpy_exec.c
> @@ -375,11 +375,11 @@ PLy_exec_trigger(FunctionCallInfo fcinfo, PLyProcedure *proc)
> rv = NULL;
> else if (pg_strcasecmp(srv, "MODIFY") == 0)
> {
> - TriggerData *tdata = (TriggerData *) fcinfo->context;
> + TriggerData *trigdata = (TriggerData *) fcinfo->context;
>
> - if (TRIGGER_FIRED_BY_INSERT(tdata->tg_event) ||
> - TRIGGER_FIRED_BY_UPDATE(tdata->tg_event))
> - rv = PLy_modify_tuple(proc, plargs, tdata, rv);
> + if (TRIGGER_FIRED_BY_INSERT(trigdata->tg_event) ||
> + TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event))
> + rv = PLy_modify_tuple(proc, plargs, trigdata, rv);
> else
> ereport(WARNING,
> (errmsg("PL/Python trigger function returned \"MODIFY\" in a DELETE trigger --
ignored")));
This doesn't strike me as a good fix either. Isn't the inner tdata exactly
the same as the outer tdata?
tdata = (TriggerData *) fcinfo->context;
...
TriggerData *trigdata = (TriggerData *) fcinfo->context;
> --- a/src/test/modules/test_integerset/test_integerset.c
> +++ b/src/test/modules/test_integerset/test_integerset.c
> @@ -585,26 +585,26 @@ test_huge_distances(void)
This is one of the cases where our insistence on -Wdeclaration-after-statement
really makes this unnecessary ugly... Declaring x at the start of the function
just makes this harder to read.
Anyway, this isn't important code, and your fix seem ok.
Greetings,
Andres Freund
Attachment
On Thu, 6 Oct 2022 at 13:39, Andres Freund <andres@anarazel.de> wrote:
> I attached a patch to add -Wshadow=compatible-local to our set of warnings.
Thanks for writing that and for looking at the patch.
FWIW, I'm +1 for having this part of our default compilation flags. I
don't want to have to revisit this on a yearly basis. I imagine Justin
doesn't want to do that either. I feel that since this work has
already uncovered 2 existing bugs that it's worth having this as a
default compilation flag. Additionally, in the cases like in the
PLy_exec_trigger() trigger case below, I feel this has resulted in
slightly more simple code that's easier to follow. I feel having to be
slightly more inventive with variable names in a small number of cases
is worth the trouble. I feel the cases where this could get annoying
are probably limited to variables declared in macros. Maybe that's
just a reason to consider static inline functions instead. That
wouldn't work for macros such as PG_TRY(), but I think macros in that
category are rare. I think switching it on does not mean we can never
switch it off again should we ever find something we're unable to work
around. That just seems a little unlikely given that with the prior
commits plus the attached patch, we've managed to fix ~30 years worth
of opportunity to introduce shadowed local variables.
> > diff --git a/contrib/hstore/hstore.h b/contrib/hstore/hstore.h
> > #define HS_FINALIZE(hsp_,count_,buf_,ptr_) \
> > do {
\
> > - int buflen = (ptr_) - (buf_); \
> > + int _buflen = (ptr_) - (buf_); \
>
> Not pretty. Given that HS_FINALIZE already has multiple-eval hazards, perhaps
> we could just remove the local?
You're right. It's not that pretty, but I don't feel like making the
hazards any worse is a good idea. This is old code. I'd rather change
it as little as possible to minimise the risk of introducing any bugs.
I'm open to other names for the variable, but I just don't want to
widen the scope for multiple evaluation hazards.
> > --- a/src/interfaces/libpq/fe-secure-gssapi.c
> > +++ b/src/interfaces/libpq/fe-secure-gssapi.c
> > @@ -135,11 +135,11 @@ pg_GSS_write(PGconn *conn, const void *ptr, size_t len)
> > - ssize_t ret;
> > + ssize_t retval;
>
> That looks like it could easily lead to confusion further down the
> line. Wouldn't the better fix here be to remove the inner variable?
hmm. You're maybe able to see something I can't there, but to me, it
looks like reusing the outer variable could change the behaviour of
the function. Note at the end of the function we set "ret" just
before the goto label. It looks like it might be possible for the
goto to jump to the point after "ret = bytes_sent;", in which case we
should return -1, the default value for the outer "ret". If I go and
reuse the outer "ret" for something else then it'll return whatever
value it's left set to. I could study the code more and perhaps work
out that that cannot happen, but if it can't then it's really not
obvious to me and if it's not obvious then I just don't feel the need
to take any undue risks by reusing the outer variable. I'm open to
better names, but I'd just rather not reuse the outer scoped variable.
> > --- a/src/pl/plpython/plpy_exec.c
> > +++ b/src/pl/plpython/plpy_exec.c
> > @@ -375,11 +375,11 @@ PLy_exec_trigger(FunctionCallInfo fcinfo, PLyProcedure *proc)
> > - TriggerData *tdata = (TriggerData *) fcinfo->context;
> > + TriggerData *trigdata = (TriggerData *) fcinfo->context;
> This doesn't strike me as a good fix either. Isn't the inner tdata exactly
> the same as the outer data?
Yeah, you're right. I've adjusted the patch to use the outer scoped
variable and get rid of the inner scoped one.
> > --- a/src/test/modules/test_integerset/test_integerset.c
> > +++ b/src/test/modules/test_integerset/test_integerset.c
> > @@ -585,26 +585,26 @@ test_huge_distances(void)
>
> This is one of the cases where our insistence on -Wdeclaration-after-statement
> really makes this unnecessary ugly... Declaring x at the start of the function
> just makes this harder to read.
Yeah, it's not pretty. Maybe one day we'll relax that rule. Until
then, I think it's not worth expending too much thought on a test
module.
David
Attachment
On 2022-Oct-06, David Rowley wrote:
> On Thu, 6 Oct 2022 at 02:34, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > A simpler idea might be to just remove the inner declaration, and have
> > that block set the outer var. There's no damage, since the block is
> > going to end and not access the previous value anymore.
> >
> > diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
> > index aa1a3541fe..91a067859b 100644
> > --- a/src/bin/pgbench/pgbench.c
> > +++ b/src/bin/pgbench/pgbench.c
> > @@ -7506,7 +7506,7 @@ threadRun(void *arg)
> > /* progress report is made by thread 0 for all threads */
> > if (progress && thread->tid == 0)
> > {
> > - pg_time_usec_t now = pg_time_now();
> > + now = pg_time_now(); /* not lazy; clobbers outer value */
>
> I didn't want to do it that way because all this code is in a while
> loop and the outer "now" will be reused after it's set by the code
> above. It's not really immediately obvious to me what repercussions
> that would have, but it didn't seem worth taking any risks.
No, it's re-initialized to zero every time through the loop, so setting
it to something else at the bottom doesn't have any further effect.
If it were *not* reinitialized every time through the loop, then what
would happen is that every iteration in the loop (and each operation
within) would see exactly the same value of "now", because it's only set
"lazily" (meaning, if already set, don't change it.)
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
On Thu, 6 Oct 2022 at 20:32, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2022-Oct-06, David Rowley wrote: > > I didn't want to do it that way because all this code is in a while > > loop and the outer "now" will be reused after it's set by the code > > above. It's not really immediately obvious to me what repercussions > > that would have, but it didn't seem worth taking any risks. > > No, it's re-initialized to zero every time through the loop, so setting > it to something else at the bottom doesn't have any further effect. Oh yeah, you're right. > If it were *not* reinitialized every time through the loop, then what > would happen is that every iteration in the loop (and each operation > within) would see exactly the same value of "now", because it's only set > "lazily" (meaning, if already set, don't change it.) On my misread, that's what I was afraid of changing, but now seeing that now = 0 at the start of each loop, I understand that pg_time_now_lazy will get an up-to-date time on each loop. I'm happy if you want to change it to use the outer scoped variable instead of the now2 one. David
On Thu, 6 Oct 2022 at 13:39, Andres Freund <andres@anarazel.de> wrote: > I attached a patch to add -Wshadow=compatible-local to our set of warnings. Since I just committed the patch to fix the final warnings, I think we should go ahead and commit the patch you wrote to add -Wshadow=compatible-local to the standard build flags. I don't mind doing this. Does anyone think we shouldn't do it? Please let it be known soon. David
On Fri, 7 Oct 2022 at 13:24, David Rowley <dgrowleyml@gmail.com> wrote: > Since I just committed the patch to fix the final warnings, I think we > should go ahead and commit the patch you wrote to add > -Wshadow=compatible-local to the standard build flags. I don't mind > doing this. Pushed. David
David Rowley <dgrowleyml@gmail.com> writes:
> On Fri, 7 Oct 2022 at 13:24, David Rowley <dgrowleyml@gmail.com> wrote:
>> Since I just committed the patch to fix the final warnings, I think we
>> should go ahead and commit the patch you wrote to add
>> -Wshadow=compatible-local to the standard build flags. I don't mind
>> doing this.
> Pushed.
The buildfarm's showing a few instances of this warning, which seem
to indicate that not all versions of the Perl headers are clean:
fairywren | 2022-10-10 09:03:50 | C:/Perl64/lib/CORE/cop.h:612:13: warning: declaration of 'av' shadows a previous
local[-Wshadow=compatible-local]
fairywren | 2022-10-10 09:03:50 | C:/Perl64/lib/CORE/cop.h:612:13: warning: declaration of 'av' shadows a previous
local[-Wshadow=compatible-local]
fairywren | 2022-10-10 09:03:50 | C:/Perl64/lib/CORE/cop.h:612:13: warning: declaration of 'av' shadows a previous
local[-Wshadow=compatible-local]
fairywren | 2022-10-10 09:03:50 | C:/Perl64/lib/CORE/cop.h:612:13: warning: declaration of 'av' shadows a previous
local[-Wshadow=compatible-local]
fairywren | 2022-10-10 09:03:50 | C:/Perl64/lib/CORE/cop.h:612:13: warning: declaration of 'av' shadows a previous
local[-Wshadow=compatible-local]
fairywren | 2022-10-10 09:03:50 | C:/Perl64/lib/CORE/cop.h:612:13: warning: declaration of 'av' shadows a previous
local[-Wshadow=compatible-local]
snakefly | 2022-10-10 08:21:05 | Util.c:457:14: warning: declaration of 'cv' shadows a parameter
[-Wshadow=compatible-local]
Before you ask:
fairywren: perl 5.24.3
snakefly: perl 5.16.3
which are a little old, but not *that* old.
Scraping the configure logs also shows that only half of the buildfarm
(exactly 50 out of 100 reporting animals) knows -Wshadow=compatible-local,
which suggests that we might see more of these if they all did. On the
other hand, animals with newer compilers probably also have newer Perl
installations, so assuming that the Perl crew have kept this clean
recently, maybe not.
Not sure if this is problematic enough to justify removing the switch.
A plausible alternative is to have a few animals with known-clean Perl
installations add the switch manually (and use -Werror), so that we find
out about violations without having warnings in the face of developers
who can't fix them. I'm willing to wait to see if anyone complains of
such warnings, though.
regards, tom lane
Hi,
On 2022-10-10 12:06:22 -0400, Tom Lane wrote:
> Scraping the configure logs also shows that only half of the buildfarm
> (exactly 50 out of 100 reporting animals) knows -Wshadow=compatible-local,
> which suggests that we might see more of these if they all did.
I think it's not just newness - only gcc has compatible-local, even very new
clang doesn't.
This was fixed ~6 years ago in perl:
commit f2b9631d5d19d2b71c1776e1193173d13f3620bf
Author: David Mitchell <davem@iabyn.com>
Date: 2016-05-23 14:43:56 +0100
CX_POP_SAVEARRAY(): use more distinctive var name
Under -Wshadow, CX_POP_SAVEARRAY's local var 'av' can generate this
warning:
warning: declaration shadows a local variable [-Wshadow]
So rename it to cx_pop_savearay_av to reduce the risk of a clash.
(See http://nntp.perl.org/group/perl.perl5.porters/236444)
> Not sure if this is problematic enough to justify removing the switch.
> A plausible alternative is to have a few animals with known-clean Perl
> installations add the switch manually (and use -Werror), so that we find
> out about violations without having warnings in the face of developers
> who can't fix them. I'm willing to wait to see if anyone complains of
> such warnings, though.
Given the age of affected perl instances I suspect there'll not be a lot of
developers affected, and the number of warnings is reasonably small too. It'd
likely hurt more developers to not see the warnings locally, given that such
shadowing often causes bugs.
Greetings,
Andres Freund
On 2022-Oct-10, Andres Freund wrote: > Given the age of affected perl instances I suspect there'll not be a lot of > developers affected, and the number of warnings is reasonably small too. It'd > likely hurt more developers to not see the warnings locally, given that such > shadowing often causes bugs. Maybe we can install a filter-out in src/pl/plperl's Makefile for the time being. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Por suerte hoy explotó el califont porque si no me habría muerto de aburrido" (Papelucho)
Hi, On 2022-10-10 18:33:11 +0200, Alvaro Herrera wrote: > On 2022-Oct-10, Andres Freund wrote: > > > Given the age of affected perl instances I suspect there'll not be a lot of > > developers affected, and the number of warnings is reasonably small too. It'd > > likely hurt more developers to not see the warnings locally, given that such > > shadowing often causes bugs. > > Maybe we can install a filter-out in src/pl/plperl's Makefile for the > time being. We could, but is it really a useful thing for something fixed 6 years ago? Greetings, Andres Freund
On 2022-10-10 09:37:38 -0700, Andres Freund wrote: > On 2022-10-10 18:33:11 +0200, Alvaro Herrera wrote: > > On 2022-Oct-10, Andres Freund wrote: > > > > > Given the age of affected perl instances I suspect there'll not be a lot of > > > developers affected, and the number of warnings is reasonably small too. It'd > > > likely hurt more developers to not see the warnings locally, given that such > > > shadowing often causes bugs. > > > > Maybe we can install a filter-out in src/pl/plperl's Makefile for the > > time being. > > We could, but is it really a useful thing for something fixed 6 years ago? As an out, a hypothetical dev could add -Wno-shadow=compatible-local to their CFLAGS.
On 2022-Oct-10, Andres Freund wrote: > On 2022-10-10 09:37:38 -0700, Andres Freund wrote: > > On 2022-10-10 18:33:11 +0200, Alvaro Herrera wrote: > > > On 2022-Oct-10, Andres Freund wrote: > > > > > > > Given the age of affected perl instances I suspect there'll not be a lot of > > > > developers affected, and the number of warnings is reasonably small too. It'd > > > > likely hurt more developers to not see the warnings locally, given that such > > > > shadowing often causes bugs. > > > > > > Maybe we can install a filter-out in src/pl/plperl's Makefile for the > > > time being. > > > > We could, but is it really a useful thing for something fixed 6 years ago? Well, for people purposefully installing using older installs of Perl (not me, admittedly), it does seem useful, because you get the benefit of checking shadow vars for the rest of the tree and still get no warnings if everything is clean. > As an out, a hypothetical dev could add -Wno-shadow=compatible-local to their > CFLAGS. But that disables it for the tree as a whole, which is not better. We can remove the filter-out when we decide to move the Perl version requirement up, say 4 years from now. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "El hombre nunca sabe de lo que es capaz hasta que lo intenta" (C. Dickens)
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2022-Oct-10, Andres Freund wrote:
>> We could, but is it really a useful thing for something fixed 6 years ago?
> Well, for people purposefully installing using older installs of Perl
> (not me, admittedly), it does seem useful, because you get the benefit
> of checking shadow vars for the rest of the tree and still get no
> warnings if everything is clean.
Meh --- people purposefully using old Perls are likely using old
compilers too. Let's wait and see if any devs actually complain.
regards, tom lane
On Tue, 11 Oct 2022 at 06:02, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > On 2022-Oct-10, Andres Freund wrote: > >> We could, but is it really a useful thing for something fixed 6 years ago? > > > Well, for people purposefully installing using older installs of Perl > > (not me, admittedly), it does seem useful, because you get the benefit > > of checking shadow vars for the rest of the tree and still get no > > warnings if everything is clean. > > Meh --- people purposefully using old Perls are likely using old > compilers too. Let's wait and see if any devs actually complain. I can't really add much here, apart from I think it would be a shame if some 3rd party 6 year old code was to hold us back on this. I'm also keen to wait for complaints and only if we really have to, remove the shadow flag from being used only in the places where we need to. Aside from this issue, if anything I'd be keen to go a little further with this and upgrade to -Wshadow=local. The reason being is that I noticed that the const qualifier is not classed as "compatible" with the equivalently named and typed variable without the const qualifier. ISTM that there's close to as much opportunity to mix up two variables with the same name that are const and non-const as there are two variables with the same const qualifier. However, I'll be waiting for the dust to settle on the current flags before thinking any more about that. David
On Tue, Oct 11, 2022 at 01:16:50PM +1300, David Rowley wrote:
> Aside from this issue, if anything I'd be keen to go a little further
> with this and upgrade to -Wshadow=local. The reason being is that I
> noticed that the const qualifier is not classed as "compatible" with
> the equivalently named and typed variable without the const qualifier.
> ISTM that there's close to as much opportunity to mix up two variables
> with the same name that are const and non-const as there are two
> variables with the same const qualifier. However, I'll be waiting for
> the dust to settle on the current flags before thinking any more about
> that.
-Wshadow=compatible-local causes one extra warning in postgres.c with
-DWRITE_READ_PARSE_PLAN_TREES:
postgres.c: In function ‘pg_rewrite_query’:
postgres.c:818:37: warning: declaration of ‘query’ shadows a parameter [-Wshadow=compatible-local]
818 | Query *query = lfirst_node(Query, lc);
| ^~~~~
postgres.c:771:25: note: shadowed declaration is here
771 | pg_rewrite_query(Query *query)
| ~~~~~~~^~~~~
Something like the patch attached would deal with this one.
--
Michael
Attachment
On Wed, 12 Oct 2022 at 14:39, Michael Paquier <michael@paquier.xyz> wrote: > -Wshadow=compatible-local causes one extra warning in postgres.c with > -DWRITE_READ_PARSE_PLAN_TREES: > postgres.c: In function ‘pg_rewrite_query’: > postgres.c:818:37: warning: declaration of ‘query’ shadows a parameter [-Wshadow=compatible-local] > 818 | Query *query = lfirst_node(Query, lc); > | ^~~~~ > postgres.c:771:25: note: shadowed declaration is here > 771 | pg_rewrite_query(Query *query) > | ~~~~~~~^~~~~ > > Something like the patch attached would deal with this one. Thanks for finding that and coming up with the patch. It looks fine to me. Do you want to push it? David
On Wed, Oct 12, 2022 at 02:50:58PM +1300, David Rowley wrote: > Thanks for finding that and coming up with the patch. It looks fine to > me. Do you want to push it? Thanks for double-checking. I'll do so shortly, I just got annoyed by that for a few days :) Thanks for your work on this thread to be able to push the switch by default, by the way. -- Michael
Attachment
On 2022-Oct-11, David Rowley wrote: > I'm also keen to wait for complaints and only if we really have to, > remove the shadow flag from being used only in the places where we > need to. +1 -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "The problem with the future is that it keeps turning into the present" (Hobbes)