From: | Noah Misch <noah(at)leadboat(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | David Rowley <dgrowleyml(at)gmail(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Remove redundant initializations |
Date: | 2021-09-13 01:26:33 |
Message-ID: | 20210913012633.GA2721984@rfd.leadboat.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
I'm +1 for the $SUBJECT concept, mostly because I take longer to read code
where immaterial zero-initialization lines are diluting the code. A quick
scan of the patch content is promising. If there's a decision to move
forward, I'm happy to review it more closely.
On Wed, Jun 30, 2021 at 09:28:17AM -0400, Tom Lane wrote:
> David Rowley <dgrowleyml(at)gmail(dot)com> writes:
> > On Tue, 29 Jun 2021 at 02:15, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> The primary case where I personally rely on that style is when adding a
> >> new field to a struct. Currently it's possible to grep for some existing
> >> field and add the new one beside it. Leaving out initializations by
> >> relying on side-effects of makeNode makes that far riskier.
>
> > FWIW, I mostly grep for makeNode(NameOfNode) as I'm a bit mistrusting
> > of if the random existing field name that I pick to grep for will
> > properly showing me all the locations I should touch.
>
> I tend to do that too, but it's not a foolproof thing either, since
> some places use random memset's for the purpose.
I checked the first five matches of "git grep ' = T_'" to get a sense of code
sites that skip makeNode(). Just one of those five initializes every field:
recordDependencyOnSingleRelExpr() builds RangeTblEntry, subset of fields
EventTriggerCommonSetup() builds EventTriggerData, full fields
validateForeignKeyConstraint() builds TriggerData, subset of fields
ExecBSInsertTriggers() builds TriggerData, subset of fields [many similar examples in trigger.c]
ExecBuildProjectionInfo() builds ExprState, subset of fields
Hence, I find we're already too inconsistent about "explicitly initialize
every field" to recommend "grep for some existing field". (Two participants
in the 2018 thread made similar observations[1][2].) grepping T_NameOfNode
and then makeNode(NameOfNode) is more reliable today, and $SUBJECT will not
decrease its reliability.
[1] https://postgr.es/m/20180830045736.p3mrugcq2j367a3l@alap3.anarazel.de
[2] https://postgr.es/m/CA+TgmoYPw3Y8ZKofseTpVbb8avy7v7JbjmG6BMe7cC+eOd7qVA@mail.gmail.com
From | Date | Subject | |
---|---|---|---|
Next Message | Kyotaro Horiguchi | 2021-09-13 01:31:07 | walsender timeout on logical replication set |
Previous Message | Justin Pryzby | 2021-09-13 01:23:44 | Re: brin multi minmax crash for inet value |